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

[hxb] Implement shared string pools #11511

Merged
merged 10 commits into from
Apr 30, 2024
Merged

[hxb] Implement shared string pools #11511

merged 10 commits into from
Apr 30, 2024

Conversation

Simn
Copy link
Member

@Simn Simn commented Jan 28, 2024

This implements what I'm talking about in #11510. When using --hxb, a top-level StringPool.hxb is generated, which is then read once and passed to all hxb readers. For now, this is always activated. Once #11507 is merged we can make this configurable. I think the default should probably be off because it's unclear if this actually yields better performance characteristics for completion and such.

The cache-aspect of this is not implemented yet. @kLabz Could you handle this? It should be very straightforward because all hxb readers have access to a cc instance anyway, so if that carries a string pool we should be good to go. The only part I'm worried about is --hxb using the data from the cache, but perhaps that is also just a matter of writing the pool from cc to StringPool.hxb.

Edit: Note that the cache version should not call StringPool.finalize because we never want finalize the persisted pool.

@kLabz
Copy link
Contributor

kLabz commented Feb 13, 2024

I still need to (at least) tackle the "--hxb using the data from the cache" part, for which I'm not sure yet what strategy to apply since macro and non macro context use different cc instances, and thus different string pools. Maybe we should write StringPool.hxb and StringPool.macro.hxb? (but that would remove quite a bit of optimization from the generated file.. which may not be an issue?)

@Simn
Copy link
Member Author

Simn commented Feb 13, 2024

Yes I think that's what the "TODO: macro vs non macro" was about.

@kLabz
Copy link
Contributor

kLabz commented Feb 13, 2024

Yeah that was me adding that when merging dev with the config changes x)

@kLabz kLabz removed their assignment Feb 13, 2024
@kLabz kLabz added this to the 5.0 preview 1 milestone Apr 29, 2024
@kLabz
Copy link
Contributor

kLabz commented Apr 29, 2024

Adding to 5.0 preview 1 because I don't remember what the status is here (thought this was merged already..) so I should at least check what's up.

@Simn
Copy link
Member Author

Simn commented Apr 29, 2024

Huh, I also thought we merged this already...

@kLabz
Copy link
Contributor

kLabz commented Apr 30, 2024

So, this lgtm.

Rudy CI (well, the parts that I updated) complains a bit about changing IDs but that applies to development too and probably isn't relevant anyway.

I see around 33% time reduction on display requests, which is pretty nice (though I'm still barely under 200ms).

@Simn
Copy link
Member Author

Simn commented Apr 30, 2024

The configuration is still missing, but I'd be fine with adding that "later".

@kLabz
Copy link
Contributor

kLabz commented Apr 30, 2024

Opened #11648 for this, I need that small perf boost to work on further improvements :)

@kLabz kLabz merged commit 6b3d9e9 into development Apr 30, 2024
99 checks passed
@kLabz kLabz deleted the hxb_shared_string_pools branch May 28, 2024 09:33
@Simn Simn mentioned this pull request Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants