-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
generator support: eliminated performance sink in 'node-processor', solves #1293 #1294
Conversation
sailingKieler
commented
Nov 17, 2023
- added content length tracking to avoid repeated string concatenation under way
…olves #1293 * added content length tracking to avoid repeated string concatenation under way
@@ -35,6 +36,10 @@ class Context { | |||
return this.lines.map(e => e.join('')).join(''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That why I prefer to have a get
or compute
in names to make it clear there are things happening. Where this.content.length
looks like a simple field access.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work @sailingKieler !!
The patch fixes the performance effect:
- Prior to the patch the performance was very slow (see Performance of CompositeGeneratorNode for large outputs #1292) and hat a O(N^2) characteristic regarding the number of generated lines (in presence of indentation).
- Now it seems to be linear (as expected) and much faster.
Timings with my example now:
nb lines | time in sec to produce input | time for toString |
---|---|---|
3'200'000 | 0.7 | 2.7 |
1'600'000 | 0.3 | 1.25 |
800'000 | 0.15 | 0.7 |
400'000 | 0.1 | 0.4 |
This is much better than before:
nb lines | time for toString |
---|---|
8'000 | 4.0 |
16'000 | 16.0 |
I can also confirm that the output is identical for the example sketched in #1292:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me as well 👍