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

perf(es/ast): Use Atom in some places #5271

Merged
merged 63 commits into from
Jul 22, 2022
Merged

perf(es/ast): Use Atom in some places #5271

merged 63 commits into from
Jul 22, 2022

Conversation

kdy1
Copy link
Member

@kdy1 kdy1 commented Jul 22, 2022

Description:

As this is tradeoff of comparison/hash time and global lock, I decided to use Atom for only places I can be sure.

Related issue (if exists):

@kdy1 kdy1 added this to the Planned milestone Jul 22, 2022
@kdy1
Copy link
Member Author

kdy1 commented Jul 22, 2022

@kwonoj As it's a type, how long is too long does not exist.
It's used unconditionally, and the important point is not likely to be duplicated, not length

kodiakhq[bot]
kodiakhq bot previously approved these changes Jul 22, 2022
@kdy1 kdy1 changed the title perf(es): Use Atom in some places perf(es/ast): Use Atom in some places Jul 22, 2022
kodiakhq[bot]
kodiakhq bot previously approved these changes Jul 22, 2022
@kdy1 kdy1 enabled auto-merge (squash) July 22, 2022 07:50
kodiakhq[bot]
kodiakhq bot previously approved these changes Jul 22, 2022
@kdy1 kdy1 disabled auto-merge July 22, 2022 11:09
@kdy1 kdy1 merged commit 037a53d into swc-project:main Jul 22, 2022
@kdy1 kdy1 deleted the atom branch July 22, 2022 11:09
@overlookmotel
Copy link
Contributor

Argh! I'm too late to the party. You move so damn fast!

I had an idea about how it might be possible to combine the advantages of JsWord and the new Atom in a single entity.

In short: have a shared-across-threads static set (read only, so no mutex required) pre-populated with common words + a per-thread dynamic set (for words not in the static set). The static set could use perfect hashes, and the dynamic set not. Both could use the faster FxHasher.

However, work is completely crazy today so it'll have to wait until weekend for me to outline this idea further. I'll open an issue as soon as I have time.

@kdy1
Copy link
Member Author

kdy1 commented Jul 22, 2022

I merged it after verifying the performance of other operations, but yeah, I only used Atom for places where we don't need perfect hashing

@overlookmotel
Copy link
Contributor

Quick thought: Since the Atoms are never shared across threads, couldn't they use Rc instead of Arc? My understanding is Rc can have less overhead.

GiveMe-A-Name pushed a commit to GiveMe-A-Name/swc that referenced this pull request Jul 23, 2022
@kdy1
Copy link
Member Author

kdy1 commented Jul 23, 2022

Nope. they are moved across threads

@overlookmotel overlookmotel mentioned this pull request Jul 23, 2022
6 tasks
@kdy1 kdy1 self-assigned this Jul 25, 2022
@kdy1 kdy1 modified the milestones: Planned, v1.2.219 Jul 27, 2022
@swc-project swc-project locked as resolved and limited conversation to collaborators Oct 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants