avoid any heap allocation for default constructed Format() as used as "invalid"
ClosedPublic

Authored by cullmann on Aug 20 2018, 7:10 PM.

Details

Summary

Format() is used as invalid return value in some places.
At the moment that does at least one heap allocation.
Now only for formats loaded from the XML a heap allocation is done

Test Plan

make && make test

Diff Detail

Repository
R216 Syntax Highlighting
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
cullmann created this revision.Aug 20 2018, 7:10 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptAug 20 2018, 7:10 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
cullmann requested review of this revision.Aug 20 2018, 7:10 PM

Unit test missing ...

There is no test missing, these code paths are anyways already tested.
;=) Otherwise I would not have found all segfaults.

But I can call all accessors on some default constructed Format() if that increases the confidence, if this patch is at all wanted.
It saves heap allocations but adds some branches.

Btw, would it also work to point to an internal static d-pointer to avoid all the d-pointer checking? The current patch is likely fine, but it feels a bit messy... But I'd agree to this if other solutions are not feasible.

I am a QExplicitlySharedDataPointer noob ;=) Perhaps Volker can tell about that ;=)

A shared static null object should work with QExplicitlySharedDataPointer as with any other d pointer type: just copy the null object into d in the default ctor. Trades the conditions per call for an atomic increment/decrement per instance.

If that is more elegant, I would be happy with this a solution, too.
The Format() allocations just showed up as a few MBs during heaptrack profiling of KWrite (just loading one large file and rehighlighting it).

I have no strong preference for either of those approaches, but I do agree with this optimization of course :)

Just for the implementation: Is the static default than just a e.g. function static QExplicitlySharedDataPointer<FormatPrivate>(new FormatPrivate()) like:

QExplicitlySharedDataPointer<FormatPrivate> &staticDefault()
{

static QExplicitlySharedDataPointer<FormatPrivate> default(new FormatPrivate());
return default;

}

and the constructor will do

Format::Format() : d(staticDefault())
{
}

or is that some other trick?

That should work, yes.

That looks much nicer an avoids the !d checks, which are tedious.

I think the one ref count change is no issue, at least that is orders of magnitude better than some malloc on Windows :)

I will adapt the patch.

cullmann added a comment.EditedAug 21 2018, 10:30 AM

Hmm, if we go that route, what to do in:

FormatPrivate* FormatPrivate::get(const Format &format)

shall one detach there always?

cullmann updated this revision to Diff 40134.Aug 21 2018, 10:37 AM

Simpler variant with shared default FormatPrivate.

vkrause accepted this revision.Aug 21 2018, 11:52 AM

Looks good to me, and is indeed a lot less code.

This revision is now accepted and ready to land.Aug 21 2018, 11:52 AM
This revision was automatically updated to reflect the committed changes.