Performance bottlenecks and findings

[Boshen]

A summary of perf bottlenecks and findings.
(I don't have time for any code work so I'll just jot down the things I find.)

General background research study:

• follow the work of https://github.com/nnethercote, read his blog, read the perf book

[Boshen]

Bounded by Memory Allocation:

• switch to jemallocator: https://nnethercote.github.io/perf-book/heap-allocations.html#using-an-alternative-allocator
• use stack allocated string where possible: https://github.com/epage/string-benchmarks-rs

  • tools/crates/rome_js_parser/src/syntax/binding.rs

    Line 121 in 76bf61e

    let identifier_name = String::from(identifier_name);

• use smallvec where possible?
• use memory arena for Parser.events?
  • https://github.com/rome/tools/blob/main/crates/rome_js_parser/src/parser.rs#L72

[Boshen]

Being on Windows I couldn't try jemalloc but I did have a branch switching to mimalloc and if I remember correctly there was a ~10% improvement to the performances (but it might have included the formatter as well which puts even more stress on the allocator than the parser does)

~50-200% improvement done in this PR

[Conaclos]

ArrayVec when possible may be a better choice than SmallVec. It does not use a fallback to heap allocation.

[ematipico]

ArrayVec when possible may be a better choice than SmallVec. It does not use a fallback to heap allocation.

Do you have some link that explains why this crate is a better choice than smallvec?

[Conaclos]

@ematipico

The heap allocation section from the perf book talks about that.

[IWANABETHATGUY]

@ematipico

The heap allocation section from the perf book talks about that.

Notice the prerequisite,

If you have many short vectors and you precisely know their maximum length, ArrayVec from the arrayvec crate is a better choice than SmallVec. It does not require the fallback to heap allocation, which makes it a little faster. Example.

If you have many short vectors and you precisely know their maximum length. The Identifier could have any length less than std::usize::MAX in theory.

[Boshen]

rustc options:
In Cargo.toml:

[profile.release]
lto = "fat"
codegen-units = 1
panic = "abort"
strip = "debuginfo"

In .cargo/config.toml

[build]
rustflags = ["-C", "target-cpu=native"]

[leops]

The lto and strip options are already set as part of the release workflow (https://github.com/rome/tools/blob/main/.github/workflows/release.yml#L108-L111), I didn't set codegen-units to one because I thought it was the default for the release profile but I might have been wrong. For the others, I'm not really sure about aborting on panics because while it may improve performance it could also degrade the user experience in case of a bug, with unwinding we can at least try to recover from these errors. Finally setting the target-cpu to native feels like cheating, it may improve performance if the benchmarks are being run on the same machine that builds them but in practice we distribute prebuilt binaries to the users, so those have to be optimized for the common case.

[Boshen]

Bounded by string comparison:

• tools/crates/rome_js_parser/src/syntax/binding.rs

  Line 72 in 76bf61e

  if StrictMode.is_supported(p) && matches!(identifier_name, "eval" | "arguments") {

  • This one is small but anything on the identifier path can impact perf

I can't tell if String Interning is needed for this project.

[leops]

I did try to add interning to identifiers in the parser, but interestingly the results I got was that it made the performance of the parser generally worse so I didn't pursue this idea further. Although I still think we will have to implement some form of interning eventually because down the line linters / analyzers will needs a cheap and efficient way to lookup and compare names in a program.

[MichaReiser]

@leops that's an interesting point and again raises the discussion of symbol resolution should or should not happen inside of the parser.

Regarding interning: Lookups in the analyzers would ideally work with a Symbol type (rather than operating on strings directly) because a name by itself isn't unique enough. It also requires knowledge about the scope. The Symbol type could then act as cheap "id" to lookup and compare symbols.

[ematipico]

Rome classic did an interesting decision. It decided to leave the symbol resolution out of the parsing. The symbol resolution was done during the the "compiling step". There's this CompilerContext class which was in charge of taking a valid AST and creating the bindings out of it. Then an analyzer would get this CompilerPath (a wrapper around the CompilerContext) and it would be able to get the bindings as needed: https://github.com/rome/tools/blob/archived-js/internal/compiler/lint/rules/js/noUnusedVariables.ts#L73

[Stupremee]

I can't tell anything about the performance improvement interning would get because we never tried it in rslint. However, if you're going to implement it, I recommend using the lasso because it's by far the best string interning library IMO. It outperforms most other interning crates, provides really cool and optionally multithreaded APIs. Another crate you can have a look at is cstree which is basically rowan with improvements, and especially they have string interning built in.

[Boshen]

Lexer: anything we can improve on the lexer will have the biggest impact.

• I doubt that unwind_loop has actual perf can, since that's from 2 years ago, rust and llvm should optimize things and this unwind_loop will block it? (This macro also bloats binary output size)

[MichaReiser]

It should be straightforward to remove unwind_loop to see if it changes the performance in a meaningful way.

I'm not entirely convinced that the lexer is the area where we can achieve the most speedup (except maybe using SIMD?). Old benchmarks reported the numbers by time spent lexing, parsing, and in the tree sink (maybe something we could add again with tracing @leops ?). These numbers showed that lexing is often the cheapest of all and the tree-sink makes up about 50% of the time.

[MichaReiser]

Related PR #2311

[Boshen]

In this PR, performance for identifier lookup is improved with a lookup table, resulting a ~5% improvement, and build size reduced by 2kb.

[Boshen]

I see there are a lot of assert_eq!, is it possible to replace them with debug_assert_eq! so they can be stripped away for release build?

[Boshen]

#2379 (comment)
No significant changes 😢