Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

perf(rome_js_semantic): POC: reduce semantic model writes #3569

Closed
wants to merge 1 commit into from

Conversation

MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented Nov 6, 2022

Improve the performance of the semantic model by writing less data:

  • Only store the nodes that can be queried by the semantic model
  • Only store declaration reads/writes once instead of twice

Yields a 5-10% perf improvement

Alternatives:

Keep declaration_all_writes and declaration_all_reads and compute declaration_all_references by concatenating the two list.

Downside: Doesn't maintain read/write order
Upside: Cheaper read of reads/writes because it doesn't require any filtering

Summary

Test Plan

@MichaReiser
Copy link
Contributor Author

!bench_analyzer

@MichaReiser
Copy link
Contributor Author

MichaReiser commented Nov 6, 2022

@xunilrj do you want to take this over as you have the most knowledge about the semantic model? My implementation works but isn't very clean. I'm also undecided on what approach is best for all_reads/all_writes/all_references.

What would be nice is if we could avoid the many nested Vecs but I haven't been able to come up with a data structure. (something like this could be interesting https://doc.rust-lang.org/beta/nightly-rustc/src/rustc_data_structures/sorted_map/index_map.rs.html#28-34). But that's something we can look into later.

@github-actions
Copy link

github-actions bot commented Nov 6, 2022

Analyzer Benchmark Results

group                 main                                   pr
-----                 ----                                   --
analyzer/css.js       1.10      2.4±0.01ms     4.9 MB/sec    1.00      2.2±0.01ms     5.4 MB/sec
analyzer/index.js     1.12      6.6±0.02ms     4.9 MB/sec    1.00      5.9±0.01ms     5.5 MB/sec
analyzer/parser.ts    1.17      7.7±0.02ms     6.3 MB/sec    1.00      6.5±0.01ms     7.5 MB/sec
analyzer/router.ts    1.15      5.6±0.01ms    10.9 MB/sec    1.00      4.9±0.02ms    12.6 MB/sec

@MichaReiser MichaReiser changed the title perf(rome_js_semantic): Use FX Hash function perf(rome_js_semantic): POC: reduce semantic model writes Nov 6, 2022
@MichaReiser MichaReiser force-pushed the perf/semantic-use-fx-hasher branch from b2831b7 to 6e0f2df Compare November 6, 2022 17:25
Base automatically changed from perf/semantic-use-fx-hasher to main November 7, 2022 09:02
Improve the performance of the semantic model by writting less data:

* Only store the nodes that can be queried by the semantic model
* Only store declaration reads/writes once instead of twice

## Alternatives:

Keep `declaration_all_writes` and `declaration_all_reads` and compute `declaration_all_references` by concatenating the two list.

Downside: Doesn't maintain read/write order
Upside: Cheaper read of reads/writes because it doesn't require any filtering
@MichaReiser MichaReiser force-pushed the perf/semantic-reduce-writes branch from 9771f1d to 86c0d9f Compare November 7, 2022 09:03
@netlify
Copy link

netlify bot commented Nov 7, 2022

Deploy Preview for docs-rometools canceled.

Name Link
🔨 Latest commit 9771f1d
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/6368c9fb95ab0d0008860d1c

@MichaReiser
Copy link
Contributor Author

!bench_analyzer

@github-actions
Copy link

github-actions bot commented Nov 7, 2022

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.02      2.3±0.02ms     5.1 MB/sec    1.00      2.2±0.01ms     5.2 MB/sec
analyzer/index.js         1.05      6.5±0.01ms     5.1 MB/sec    1.00      6.2±0.01ms     5.3 MB/sec
analyzer/lint.ts          1.11      2.6±0.02ms    15.9 MB/sec    1.00      2.3±0.02ms    17.7 MB/sec
analyzer/parser.ts        1.08      7.4±0.01ms     6.6 MB/sec    1.00      6.8±0.02ms     7.1 MB/sec
analyzer/router.ts        1.05      5.4±0.01ms    11.4 MB/sec    1.00      5.1±0.15ms    12.0 MB/sec
analyzer/statement.ts     1.03      7.9±0.11ms     4.5 MB/sec    1.00      7.7±0.01ms     4.6 MB/sec
analyzer/typescript.ts    1.03     12.4±0.03ms     4.4 MB/sec    1.00     12.0±0.02ms     4.5 MB/sec

@@ -101,7 +112,50 @@ impl Visitor for SemanticModelBuilderVisitor {
) {
match event {
WalkEvent::Enter(node) => {
self.builder.push_node(node);
match node.kind() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good idea.

@xunilrj
Copy link
Contributor

xunilrj commented Nov 7, 2022

Only store declaration reads/writes once instead of twice

I thought this initially. Not sure if this makes a huge difference. Will test here.

The picking of the nodes I liked. Makes sense.

@MichaReiser
Copy link
Contributor Author

I thought this initially. Not sure if this makes a huge difference. Will test here.

I think it roughly showed 2% improvement for each removed collection (so 4% in total)

@github-actions
Copy link

This PR is stale because it has been open 14 days with no activity.

@xunilrj
Copy link
Contributor

xunilrj commented Nov 23, 2022

I incorporated these suggestions at #3825.
I think we can close this PR.

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

Successfully merging this pull request may close these issues.

2 participants