Optimize find referenced elements #167
Merged
+23
−21
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Split the handling of referencingProps into a separate loop that calls findReferencingProperty directly. This saves a bunch of "make list, join list, append to another list and eventually split text into two elements" operations. This gives approximately 10% faster runtimes on 341 kB flamegraph generated by the "nytprof" Perl profiler. Signed-off-by: Niels Thykier <niels@thykier.net>
The removeUnusedDefs function does not actually remove anything (that is left for its callers to do). This implies that findReferencedElements will return the same value before, during and after a call to removeUnusedDefs. Therefore, we can reuse the value from findReferencedElements when recursing into child nodes. Signed-off-by: Niels Thykier <niels@thykier.net>
It was a dict with a two element list a la:
{
"id1": [len(nodeListX), nodeListX]],
"id2": [len(nodeListY), nodeListY]],
...
}
This can trivially be simplified to:
{
"id1": nodeListX,
"id2": nodeListY,
...
}
The two call-sites that actually needs the length (e.g. to sort by how
often the id is used) can trivially compute that via a call to "len".
All other call sites either just need to tell if an ID is used at all
or work the nodes referencing the id (e.g. to remap the id). The
former are unaffected by this change and the latter can now avoid a
layer of indirection.
This refactoring has negiable changes to the runtime and probably also
to memory (not tested, but it is a minor constant improvement per
referenced id).
Signed-off-by: Niels Thykier <niels@thykier.net>
Codecov Report
@@ Coverage Diff @@
## master #167 +/- ##
==========================================
- Coverage 91.49% 91.48% -0.01%
==========================================
Files 5 5
Lines 2139 2138 -1
==========================================
- Hits 1957 1956 -1
Misses 182 182
Continue to review full report at Codecov.
|
|
Perfect! 6% speedup also confirmed in a class of files I'm currently working with. |
|
Thanks for the review and merge. :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The biggest win (at least in my test case) is the "findReferencedElements: Handle referencingProps separately", which avoids a large number of unnecessary string operations.
But there is also a large potential in "Avoid recomputing findReferencedElements in removeUnusedDefs" for any SVG that need a number of calls to "removeUnreferencedElements" or where "removeUnusedDefs" recurses a lot of times. My particular test case does not seem to trigger any of that though (with a total of one call to removeUnreferencedElements)