Skip to content
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

Cache head getter in PathExpressionImplV1 #1344

Merged
merged 1 commit into from
Oct 4, 2021

Conversation

chriskrycho
Copy link
Contributor

This is one of the hot paths during normalization, and it in turn calls the hottest paths of normalization (hbsPosFor and hbsCharFor). Adding this cache improves performance againt the baseline by nearly 50% of the current delta:

variant average min max
@glimmer/compiler@0.65.3 6901.4 6812 6944
@glimmer/compiler@master 9000.4 8925 9041
@glimmer/compiler@experiment 8144.8 8078 8240

This is one of the hot paths during normalization, and it in turn calls
the *hottest* paths of normalization (`hbsPosFor` and `hbsCharFor`).
Adding this cache improves performance againt the baseline by nearly 50%
of the current delta:

|           variant            | average | min  | max  |
| ---------------------------- | ------- | ---- | ---- |
| @glimmer/compiler@0.65.3     | 6901.4  | 6812 | 6944 |
| @glimmer/compiler@master     | 9000.4  | 8925 | 9041 |
| @glimmer/compiler@experiment | 8144.8  | 8078 | 8240 |
@chriskrycho
Copy link
Contributor Author

Note: I tried this and #1345 and then the combination of them, and this appears to be the winner: having this cache in place accounts for all the wins even when both are combined—which makes sense, because this change means that the other path is simply invoked much less.

For reference, below is the same comparison table I posted to #1345. (Runtimes here are lower than in the table above because I switched to running the benchmark in sequence to avoid CPU contention interfering. That was also the cause of the wild swings observed back in #1341.)

syntax-cache-head (#1344, this PR)

variant average min max
@glimmer/compiler@0.65.3 2339.2 2306 2362
@glimmer/compiler@master 3700.4 3680 3739
@glimmer/compiler@experiment 3052.8 3028 3082

syntax-cache-hbsPos (#1345)

variant average min max
@glimmer/compiler@0.65.3 2381.2 2328 2483
@glimmer/compiler@master 3750.6 3674 3939
@glimmer/compiler@experiment 3156.6 3137 3200

combined

variant average min max
@glimmer/compiler@0.65.3 2326.6 2320 2333
@glimmer/compiler@master 3710 3692 3742
@glimmer/compiler@experiment 3040.4 3002 3064

@chriskrycho
Copy link
Contributor Author

Notably, to my original summary comment on #1345, this has a nearly identical impact on call time spent in hbsPosFor, which also makes sense: we simply call it much less. If we can find a similar optimization point higher in the call tree to avoid calling charPosFor, we may get a similar win there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants