decrease StateData space by more than 50% and half the number of needed mallocs
ClosedPublic

Authored by cullmann on Aug 28 2018, 9:52 PM.

Details

Summary

less space used by the object itself and only one vector instead of two => half of the needed allocations

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 28 2018, 9:52 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptAug 28 2018, 9:52 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
cullmann requested review of this revision.Aug 28 2018, 9:52 PM
dhaumann accepted this revision.Aug 28 2018, 9:57 PM

Lgtm.

This revision is now accepted and ready to land.Aug 28 2018, 9:57 PM
cullmann updated this revision to Diff 40584.Aug 28 2018, 10:03 PM

compare of references works without diff, just missing diff output
this avoid spawning processes at all if all tests are fine

> nicer to use unit test (as long as they work) for perf

I improved the unit tests, too.
No need to call diff if the files are really equal, improves testing speed + makes the tests much nicer to use as benchmarks.

This revision was automatically updated to reflect the committed changes.
cullmann reopened this revision.Aug 28 2018, 10:28 PM

Hmpf, thought tests did work :P But memory faults are hard.

This revision is now accepted and ready to land.Aug 28 2018, 10:28 PM
This revision was automatically updated to reflect the committed changes.
cullmann reopened this revision.Aug 28 2018, 10:31 PM

I fixed the tests again by adding the m_defData back.
I am not sure, if that really is the right fix.
That was like this before, but can't by accident the refData just point again to a new definition object that is newly allocated?
Should we use a DefinitionRef there?

This revision is now accepted and ready to land.Aug 28 2018, 10:31 PM

I can't answer the m_defData question without digging deeper into this unfortunately. The rest looks good to me though.

src/lib/state.cpp
113

last() has a non-const (detaching) overload, is there a risk we are hitting this here due to the d-> indirection? All other occurrences seem to be clearly on const members.

For: I can't answer the m_defData question without digging deeper into this unfortunately. The rest looks good to me though.

Ok. I think in 99% of the cases it is save enough, thought some 100% save thing would be nicer. Actually one could just add a shared::ptr to keep the definition alive to the state.

For the indentationBasedFoldingEnabled() detach: Actually, do we want to have the implicit stuff at all there?
I would tend to just use a std::vector as stack as we do the sharing ourself (and ensure we return pointer equal states in highlightLine)

Regarding implicit sharing, is this actually shared between two (subsequent) lines with the same state (think of e.g. a multi-line license header block)? Then it might be interesting to keep it, otherwise I agree, the increased control of a std::vector might be preferable here indeed.

Regarding implicit sharing, is this actually shared between two (subsequent) lines with the same state (think of e.g. a multi-line license header block)? Then it might be interesting to keep it, otherwise I agree, the increased control of a std::vector might be preferable here indeed.

At the moment, KTextEditor compares the returned states and reuses the input state.
I would do the same in highlightLine, then you get that for free.

cullmann updated this revision to Diff 40621.Aug 29 2018, 8:01 AM

Proposed solution that is "safe": We use a weak reference to check for state validity.
This is bit more expensive than the raw-pointer but even save if e.g. by accident a new definition gets allocated to the same address as an old one.
I added shortcut to check for pointer equality for shared states.

This seems reasonable, the raw pointer use here probably predates DefinitionRef.

This revision was automatically updated to reflect the committed changes.

For the std::vector or other things, I will create new requests.