refactor(parser): remove TokenValue::BigInt from Token

[Boshen]

This PR partially fixes #1803 and is part of #1880.

BigInt is removed from the Token value, so that the token size can be
reduced once we removed all the variants.

Token is now also Copy, which removes all the clone and drop calls.

This yields 5% performance improvement for the parser.

[Boshen]

Current dependencies on/for this PR:

• main
  • PR refactor(parser): remove TokenValue::BigInt from Token #1924 👈
    • PR refactor(parser): remove TokenValue::RegExp from Token #1926

This stack of pull requests is managed by Graphite.

[codspeed-hq]

CodSpeed Performance Report

Merging #1924 will not alter performance

_{Comparing 01-07-refactor_parser_parse_BigInt_lazily (dad94f7) with main (c6eb519)}

Summary

✅ 14 untouched benchmarks

[Boshen]

cc @overlookmotel checkout https://codspeed.io/oxc-project/oxc/branches/01-07-refactor_parser_parse_BigInt_lazily

[Boshen]

[overlookmotel]

Very nice performance gain! I assume that's due to reduced size of Token (and possibly also Token now being Copy and non-drop).

However...

I don't think BigintLiteral now containing a Box<BigInt> rather than just BigInt solves the memory leak.

To explain my thinking...

oxc_allocator::Box only contains a reference &'alloc mut T, not an owned T. So (if I understand right) it has no implicit Drop impl. And oxc_allocator::Box does not have an explicit Drop impl either.

This is different to Bumpalo's Box which ensures the content of the Box is dropped when the Box is with an explicit Drop impl:
https://docs.rs/bumpalo/latest/src/bumpalo/boxed.rs.html#337-344

So you could fix the memory leak by adding a Drop impl to oxc_allocator::Box. But I don't think you want to do that! If Box is drop, then Expression and Statement become drop, which means Program is drop too. So it would have the sizeable negative consequence that dropping an AST then involves traversing the entire AST to hunt for any Boxes which contain drop types.

The fact that it doesn't do this is I think a significant factor in why OXC is so fast, and I had assumed oxc_allocator::Box was designed this way intentionally to achieve this.

The only viable solution I can see to the memory leak, which preserves OXC's speed, is to replace BigInt with an ArenaBigInt<'a> which stores its internal content in the arena - so then there's no data stored external to the arena that needs to be dropped.

Therefore, I don't think the change from BigInt to Box<BigInt> gains anything, and is in fact marginally less performant than before (not that it matters hugely because bigints are so rare in JS code).

All the (very significant) gain in this PR is due to the other change - removing BigInt from TokenValue.

Or am I misunderstanding?

[overlookmotel]

By the way, if just removing Bigint from TokenValue gets a 5% speedup, that's a very positive sign that removing TokenValue entirely is going to be MASSIVE.

[Boshen]

I copied the arena implementation from jsparagus https://github.com/mozilla-spidermonkey/jsparagus/blob/master/crates/ast/src/arena.rs

We'll have to experiment with the drop problem. oxc is being integrated into Rolldown (an incremental compiler), we are going to get into real trouble if memory leaks 💥

---

You're right about Box<BigInt>, let me drop the box from this PR for now, as it adds no functionality.