Skip to content

refactor(semantic)!: Do not expose CompactStr from Reference#3985

Closed
DonIsaac wants to merge 1 commit intomainfrom
don/semantic/refactor/name-api
Closed

refactor(semantic)!: Do not expose CompactStr from Reference#3985
DonIsaac wants to merge 1 commit intomainfrom
don/semantic/refactor/name-api

Conversation

@DonIsaac
Copy link
Contributor

@DonIsaac DonIsaac commented Jun 30, 2024

Changes Reference::name() to return a &str instead of a &CompactStr. As such, it is technically a breaking change. I scanned through Rolldown to see how they use References, and I do not think this PR will break anything.

Doing this will give us flexibility to change the representation of a reference name in the future. I'm thinking of making this an Rc<CompactStr> or something similar to reduce allocations within semantic.

@graphite-app
Copy link
Contributor

graphite-app bot commented Jun 30, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions bot added A-linter Area - Linter A-semantic Area - Semantic labels Jun 30, 2024
@codspeed-hq
Copy link

codspeed-hq bot commented Jun 30, 2024

CodSpeed Performance Report

Merging #3985 will not alter performance

Comparing don/semantic/refactor/name-api (cf7913f) with main (b257d53)

Summary

✅ 28 untouched benchmarks

@Dunqing
Copy link
Member

Dunqing commented Jul 1, 2024

image

It seems to have some impact on performance

@Boshen
Copy link
Member

Boshen commented Jul 1, 2024

Handling out CompactStr is performant here because majority of the symbols are inlined short identifiers.

@Boshen Boshen closed this Jul 1, 2024
@overlookmotel
Copy link
Member

@DonIsaac Just to say, in my view we shouldn't be using CompactString / CompactStr at all in Semantic. We could just use Atom and avoid conversion costs to/from CompactStr.

Please see oxc-project/backlog#32 which is a work-in-progress shopping list of various optimizations we could make to semantic. But... my intent is to review all the code which uses Semantic, try to think about its design "holistically", and then write up a RFC proposal to revamp it. I hope to do that later this month, or more likely next month.

There's plenty of room for improvement, but in my view, at this point it'll be more fruitful to tackle it as a whole with a defined roadmap, rather than making lots of incremental changes.

However... if you're keen to get busy on this, I'm pretty sure replacing CompactStr with Atom in semantic will be a good idea. If you'd like to do that, please open an issue for it and make sure it's agreed that's a good way forwards (I think it is, but others may disagree with me) before spending your time doing a PR.

@overlookmotel
Copy link
Member

Oh I just saw #3972 and #3974 (am catching up on issues in reverse order). Damn! Clearly this needs more thought...

@Boshen Boshen deleted the don/semantic/refactor/name-api branch September 11, 2024 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-linter Area - Linter A-semantic Area - Semantic

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants