deps: V8: cherry-pick cb00db4dba6c #48576

RaisinTen · 2023-06-27T11:23:23Z

Original commit message:

[compiler] fix CompileFunction ignoring kEagerCompile

v8::ScriptCompiler::CompileFunction was ignoring kEagerCompile. Unlike
the other functions in v8::ScriptCompiler, it was not actually
propagating kEagerCompile to the parser. The newly updated test fails
without this change.

I did some archeology and found that this was commented out since the
original CL in https://crrev.com/c/980944.

As far as I know Node.js is the main consumer of this particular API.
This CL speeds up Node.js's overall startup time by ~13%.

Change-Id: Ifc3cd6653555194d46ca48db14f7ba7a4afe0053
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/ /4571822
Commit-Queue: Marja Hölttä <[email protected]>
Reviewed-by: Marja Hölttä <[email protected]>
Cr-Commit-Position: refs/heads/main@{#87944}

Refs: v8/v8@cb00db4
Refs: #48191 (comment)

RaisinTen · 2023-06-27T14:40:21Z

test/fixtures/errors/force_colors.snapshot

@@ -5,9  5,9 @@ throw new Error('Should include grayed stack trace')
 Error: Should include grayed stack trace
    at Object.<anonymous> [90m(/[39mtest*force_colors.js:1:7[90m)[39m
 [90m    at Module._compile (node:internal*modules*cjs*loader:1257:14)[39m
-[90m    at Module._extensions..js (node:internal*modules*cjs*loader:1311:10)[39m
+[90m    at Object..js (node:internal*modules*cjs*loader:1311:10)[39m


@kvakil test/parallel/test-node-output-errors.mjs was failing, so I had to update these to pass the test. Could you please confirm if this is expected behavior?

I don't know, sorry.

kvakil

When I tried this earlier I saw that it would increase the size of the code cache / binary by quite a bit. I'm not sure what our thinking around binary size is.

I guess if binary size is important to us, we could land it, and then find the modules with the biggest code caches and switch them to non-eager compiles?

kvakil · 2023-06-28T16:53:15Z

test/fixtures/errors/force_colors.snapshot

@@ -5,9  5,9 @@ throw new Error('Should include grayed stack trace')
 Error: Should include grayed stack trace
    at Object.<anonymous> [90m(/[39mtest*force_colors.js:1:7[90m)[39m
 [90m    at Module._compile (node:internal*modules*cjs*loader:1257:14)[39m
-[90m    at Module._extensions..js (node:internal*modules*cjs*loader:1311:10)[39m
+[90m    at Object..js (node:internal*modules*cjs*loader:1311:10)[39m


I don't know, sorry.

kvakil · 2023-06-28T16:57:17Z

Also, did this produce an improvement when you tried it for the SEA bytecode caching?

Original commit message: [compiler] fix CompileFunction ignoring kEagerCompile v8::ScriptCompiler::CompileFunction was ignoring kEagerCompile. Unlike the other functions in v8::ScriptCompiler, it was not actually propagating kEagerCompile to the parser. The newly updated test fails without this change. I did some archeology and found that this was commented out since the original CL in https://crrev.com/c/980944. As far as I know Node.js is the main consumer of this particular API. This CL speeds up Node.js's overall startup time by ~13%. Change-Id: Ifc3cd6653555194d46ca48db14f7ba7a4afe0053 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/ /4571822 Commit-Queue: Marja Hölttä <[email protected]> Reviewed-by: Marja Hölttä <[email protected]> Cr-Commit-Position: refs/heads/main@{#87944} Refs: v8/v8@cb00db4 Refs: nodejs#48191 (comment) Signed-off-by: Darshan Sen <[email protected]>

Refs: https://github.com/nodejs/node/actions/runs/5389187676/jobs/9782806939?pr=48576#step:6:5624 Signed-off-by: Darshan Sen <[email protected]>

RaisinTen · 2023-07-03T08:44:29Z

@kvakil

When I tried this earlier I saw that it would increase the size of the code cache / binary by quite a bit.

Indeed, when I try to generate the SEA prep blob with the yarn cjs bundle as input (2.6M), the blob generated without this fix is 3.0M but the one with this fix is 9.1M.

I'm not sure what our thinking around binary size is.

Since the default Node.js binary itself is already big (92M on macOS), I'm not really sure if users would really mind it. I guess we could depend on some user feedback to understand if this needs to be fixed?

I guess if binary size is important to us, we could land it, and then find the modules with the biggest code caches and switch them to non-eager compiles?

Not sure I fully understand, are you referring to the modules in the input code? Currently, the requirement is that the input code needs to be bundled into a single module before generating the SEA prep blob, so when the code that generates the code cache runs, there would only be a single input module it can compile eagerly / non-eagerly.

Also, did this produce an improvement when you tried it for the SEA bytecode caching?

Without this fix, the improvement on #48191 was 24% and with this fix, it changed to 33%.

targos · 2023-07-03T09:10:32Z

Note that since this fix landed upstream, it will be applied to Node.js automatically when V8 gets upgraded.

RaisinTen · 2023-07-03T11:02:45Z

I have no idea why the coverage runs are consistently failing for this change :/

Seems to be marking large chunks of code uncovered in the report which sounds like false data:

-----------------------------------|---------|----------|---------|---------|-----------------------
File                               | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s     
-----------------------------------|---------|----------|---------|---------|-----------------------
All files                          |   69.61 |    71.53 |   62.39 |   69.61 |                       
 lib                               |   68.79 |    64.79 |   64.25 |   68.79 |                       
  _http_agent.js                   |   85.76 |    59.25 |     100 |   85.76 | ...78-479,499-500,539 
...

Lines 78 to 479 in lib/_http_agent.js are covered by our tests because these are colored green in https://app.codecov.io/gh/nodejs/node/blob/main/lib/_http_agent.js#L78.

Could this be a regression?

joyeecheung · 2023-07-03T11:53:38Z

Since the default Node.js binary itself is already big (92M on macOS), I'm not really sure if users would really mind it. I guess we could depend on some user feedback to understand if this needs to be fixed?

I looked into the binary size some time ago, and about 46% of the size comes from the ICU data (it was smaller when we only had small-ICU, and we doubled the size of the binary after we enabled full-ICU by default). The rest of the increase of the binary size over the years mostly come from the bloating of the core, especially the increased amount of JavaScript sources (though the native side also bloated to a lesser extent), which is one of my motivation to implement #46997 so that we can explore better compression options (zlib is too slow). I am working on a prototype that enables compression of the RO data (JS source / code cache / snapshot etc.) with lz4/ztsd/others and investigate which options are better/whether it's good enough to be enabled by default. Although in general I would say the increase from the code cache seems trivial compared to the size of the ICU data...

richardlau · 2023-07-03T12:11:28Z

I have no idea why the coverage runs are consistently failing for this change :/

According the the GH logs (which you have to download because they're truncated in the web UI), there are test failures

2023-07-03T10:02:34.7337054Z ===
2023-07-03T10:02:34.7337116Z === 21 tests failed
2023-07-03T10:02:34.7337188Z === 1 tests CRASHED
2023-07-03T10:02:34.7337246Z ===
2023-07-03T10:02:34.7337253Z 
2023-07-03T10:02:34.7337324Z Failed tests:
2023-07-03T10:02:34.7337664Z out/Release/node --expose-internals /home/runner/work/node/node/test/parallel/test-bootstrap-modules.js
2023-07-03T10:02:34.7337921Z out/Release/node /home/runner/work/node/node/test/parallel/test-debugger-profile.js
2023-07-03T10:02:34.7338211Z out/Release/node /home/runner/work/node/node/test/parallel/test-inspector-wait-for-connection.js
2023-07-03T10:02:34.7338643Z out/Release/node --experimental-shadow-realm --max-old-space-size=20 /home/runner/work/node/node/test/parallel/test-shadow-realm-gc.js
2023-07-03T10:02:34.7338917Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-api.js
2023-07-03T10:02:34.7339169Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-argv1.js
2023-07-03T10:02:34.7339417Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-console.js
2023-07-03T10:02:34.7339665Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-cjs-main.js
2023-07-03T10:02:34.7339980Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-dns-lookup-localhost-promise.js
2023-07-03T10:02:34.7340269Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-dns-lookup-localhost.js
2023-07-03T10:02:34.7340585Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-dns-resolve-localhost-promise.js
2023-07-03T10:02:34.7340877Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-dns-resolve-localhost.js
2023-07-03T10:02:34.7341117Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-eval.js
2023-07-03T10:02:34.7341355Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-gzip.js
2023-07-03T10:02:34.7341661Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-namespaced-builtin.js
2023-07-03T10:02:34.7341928Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-incompatible.js
2023-07-03T10:02:34.7342162Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-net.js
2023-07-03T10:02:34.7342416Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-typescript.js
2023-07-03T10:02:34.7342644Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-umd.js
2023-07-03T10:02:34.7342885Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-warning.js
2023-07-03T10:02:34.7343149Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-weak-reference.js

I'm not sure those would explain the quoted drop in coverage.

Could this be a regression?

I'd imagine that is likely. Coverage data is provided by V8 after all.

RaisinTen · 2023-07-06T06:10:05Z

Posted about the test failures in https://chromium-review.googlesource.com/c/v8/v8/ /4571822/comments/1f25968d_d8669bb9, closing

It wasn't doing anything, and actually enabling it would cause some tests to fail. Refs: nodejs#48576

It wasn't doing anything, and actually enabling it would cause some tests to fail. Refs: #48576 PR-URL: #48671 Refs: v8/v8@cb00db4 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

Original commit message: [compiler] fix CompileFunction ignoring kEagerCompile v8::ScriptCompiler::CompileFunction was ignoring kEagerCompile. Unlike the other functions in v8::ScriptCompiler, it was not actually propagating kEagerCompile to the parser. The newly updated test fails without this change. I did some archeology and found that this was commented out since the original CL in https://crrev.com/c/980944. As far as I know Node.js is the main consumer of this particular API. This CL speeds up Node.js's overall startup time by ~13%. Change-Id: Ifc3cd6653555194d46ca48db14f7ba7a4afe0053 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/ /4571822 Commit-Queue: Marja Hölttä <[email protected]> Reviewed-by: Marja Hölttä <[email protected]> Cr-Commit-Position: refs/heads/main@{#87944} Refs: v8/v8@cb00db4 PR-URL: #48671 Refs: #48576 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

It wasn't doing anything, and actually enabling it would cause some tests to fail. Refs: nodejs#48576 PR-URL: nodejs#48671 Refs: v8/v8@cb00db4 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

Original commit message: [compiler] fix CompileFunction ignoring kEagerCompile v8::ScriptCompiler::CompileFunction was ignoring kEagerCompile. Unlike the other functions in v8::ScriptCompiler, it was not actually propagating kEagerCompile to the parser. The newly updated test fails without this change. I did some archeology and found that this was commented out since the original CL in https://crrev.com/c/980944. As far as I know Node.js is the main consumer of this particular API. This CL speeds up Node.js's overall startup time by ~13%. Change-Id: Ifc3cd6653555194d46ca48db14f7ba7a4afe0053 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/ /4571822 Commit-Queue: Marja Hölttä <[email protected]> Reviewed-by: Marja Hölttä <[email protected]> Cr-Commit-Position: refs/heads/main@{#87944} Refs: v8/v8@cb00db4 PR-URL: nodejs#48671 Refs: nodejs#48576 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

It wasn't doing anything, and actually enabling it would cause some tests to fail. Refs: #48576 PR-URL: #48671 Refs: v8/v8@cb00db4 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

Original commit message: [compiler] fix CompileFunction ignoring kEagerCompile v8::ScriptCompiler::CompileFunction was ignoring kEagerCompile. Unlike the other functions in v8::ScriptCompiler, it was not actually propagating kEagerCompile to the parser. The newly updated test fails without this change. I did some archeology and found that this was commented out since the original CL in https://crrev.com/c/980944. As far as I know Node.js is the main consumer of this particular API. This CL speeds up Node.js's overall startup time by ~13%. Change-Id: Ifc3cd6653555194d46ca48db14f7ba7a4afe0053 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/ /4571822 Commit-Queue: Marja Hölttä <[email protected]> Reviewed-by: Marja Hölttä <[email protected]> Cr-Commit-Position: refs/heads/main@{#87944} Refs: v8/v8@cb00db4 PR-URL: #48671 Refs: #48576 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

It wasn't doing anything, and actually enabling it would cause some tests to fail. Refs: nodejs#48576 PR-URL: nodejs#48671 Refs: v8/v8@cb00db4 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

Original commit message: [compiler] fix CompileFunction ignoring kEagerCompile v8::ScriptCompiler::CompileFunction was ignoring kEagerCompile. Unlike the other functions in v8::ScriptCompiler, it was not actually propagating kEagerCompile to the parser. The newly updated test fails without this change. I did some archeology and found that this was commented out since the original CL in https://crrev.com/c/980944. As far as I know Node.js is the main consumer of this particular API. This CL speeds up Node.js's overall startup time by ~13%. Change-Id: Ifc3cd6653555194d46ca48db14f7ba7a4afe0053 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/ /4571822 Commit-Queue: Marja Hölttä <[email protected]> Reviewed-by: Marja Hölttä <[email protected]> Cr-Commit-Position: refs/heads/main@{#87944} Refs: v8/v8@cb00db4 PR-URL: nodejs#48671 Refs: nodejs#48576 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

It wasn't doing anything, and actually enabling it would cause some tests to fail. Refs: nodejs#48576 PR-URL: nodejs#48671 Refs: v8/v8@cb00db4 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

Original commit message: [compiler] fix CompileFunction ignoring kEagerCompile v8::ScriptCompiler::CompileFunction was ignoring kEagerCompile. Unlike the other functions in v8::ScriptCompiler, it was not actually propagating kEagerCompile to the parser. The newly updated test fails without this change. I did some archeology and found that this was commented out since the original CL in https://crrev.com/c/980944. As far as I know Node.js is the main consumer of this particular API. This CL speeds up Node.js's overall startup time by ~13%. Change-Id: Ifc3cd6653555194d46ca48db14f7ba7a4afe0053 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/ /4571822 Commit-Queue: Marja Hölttä <[email protected]> Reviewed-by: Marja Hölttä <[email protected]> Cr-Commit-Position: refs/heads/main@{#87944} Refs: v8/v8@cb00db4 PR-URL: nodejs#48671 Refs: nodejs#48576 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

It wasn't doing anything, and actually enabling it would cause some tests to fail. Refs: #48576 PR-URL: #48671 Refs: v8/v8@cb00db4 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

Original commit message: [compiler] fix CompileFunction ignoring kEagerCompile v8::ScriptCompiler::CompileFunction was ignoring kEagerCompile. Unlike the other functions in v8::ScriptCompiler, it was not actually propagating kEagerCompile to the parser. The newly updated test fails without this change. I did some archeology and found that this was commented out since the original CL in https://crrev.com/c/980944. As far as I know Node.js is the main consumer of this particular API. This CL speeds up Node.js's overall startup time by ~13%. Change-Id: Ifc3cd6653555194d46ca48db14f7ba7a4afe0053 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/ /4571822 Commit-Queue: Marja Hölttä <[email protected]> Reviewed-by: Marja Hölttä <[email protected]> Cr-Commit-Position: refs/heads/main@{#87944} Refs: v8/v8@cb00db4 PR-URL: #48671 Refs: #48576 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

It wasn't doing anything, and actually enabling it would cause some tests to fail. Refs: #48576 PR-URL: #48671 Refs: v8/v8@cb00db4 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

Original commit message: [compiler] fix CompileFunction ignoring kEagerCompile v8::ScriptCompiler::CompileFunction was ignoring kEagerCompile. Unlike the other functions in v8::ScriptCompiler, it was not actually propagating kEagerCompile to the parser. The newly updated test fails without this change. I did some archeology and found that this was commented out since the original CL in https://crrev.com/c/980944. As far as I know Node.js is the main consumer of this particular API. This CL speeds up Node.js's overall startup time by ~13%. Change-Id: Ifc3cd6653555194d46ca48db14f7ba7a4afe0053 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/ /4571822 Commit-Queue: Marja Hölttä <[email protected]> Reviewed-by: Marja Hölttä <[email protected]> Cr-Commit-Position: refs/heads/main@{#87944} Refs: v8/v8@cb00db4 PR-URL: #48671 Refs: #48576 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

It wasn't doing anything, and actually enabling it would cause some tests to fail. Refs: #48576 PR-URL: #48671 Refs: v8/v8@cb00db4 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

Original commit message: [compiler] fix CompileFunction ignoring kEagerCompile v8::ScriptCompiler::CompileFunction was ignoring kEagerCompile. Unlike the other functions in v8::ScriptCompiler, it was not actually propagating kEagerCompile to the parser. The newly updated test fails without this change. I did some archeology and found that this was commented out since the original CL in https://crrev.com/c/980944. As far as I know Node.js is the main consumer of this particular API. This CL speeds up Node.js's overall startup time by ~13%. Change-Id: Ifc3cd6653555194d46ca48db14f7ba7a4afe0053 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/ /4571822 Commit-Queue: Marja Hölttä <[email protected]> Reviewed-by: Marja Hölttä <[email protected]> Cr-Commit-Position: refs/heads/main@{#87944} Refs: v8/v8@cb00db4 PR-URL: #48671 Refs: #48576 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

This comment was marked as outdated.

Sign in to view

nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Jun 27, 2023

RaisinTen mentioned this pull request Jun 27, 2023

sea: add support for V8 bytecode-only caching #48191

Merged

This comment was marked as outdated.

Sign in to view

RaisinTen commented Jun 27, 2023

View reviewed changes

github-actions bot mentioned this pull request Jun 28, 2023

CI Reliability 2023-06-28 nodejs/reliability#601

Open

36 tasks

joyeecheung approved these changes Jun 28, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 28, 2023

This comment was marked as outdated.

Sign in to view

kvakil reviewed Jun 28, 2023

View reviewed changes

This was referenced Jun 29, 2023

CI Reliability 2023-06-29 nodejs/reliability#602

Open

CI Reliability 2023-06-30 nodejs/reliability#603

Open

RaisinTen added 2 commits June 30, 2023 18:17

test: fix test/fixtures/errors/force_colors.snapshot

4a67b73

Refs: https://github.com/nodejs/node/actions/runs/5389187676/jobs/9782806939?pr=48576#step:6:5624 Signed-off-by: Darshan Sen <[email protected]>

This was referenced Jul 1, 2023

CI Reliability 2023-07-01 nodejs/reliability#604

Open

CI Reliability 2023-07-02 nodejs/reliability#605

Open

CI Reliability 2023-07-03 nodejs/reliability#606

Open

RaisinTen force-pushed the v8/backport-kEagerCompile-fix branch from 3e73464 to 4a67b73 Compare July 3, 2023 07:27

RaisinTen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 3, 2023

This was referenced Jul 4, 2023

CI Reliability 2023-07-04 nodejs/reliability#607

Open

CI Reliability 2023-07-05 nodejs/reliability#608

Open

CI Reliability 2023-07-06 nodejs/reliability#609

Open

RaisinTen closed this Jul 6, 2023

RaisinTen deleted the v8/backport-kEagerCompile-fix branch July 6, 2023 06:10

kvakil added a commit to kvakil/node that referenced this pull request Jul 6, 2023

src: remove kEagerCompile for CompileFunction

81a2cfe

It wasn't doing anything, and actually enabling it would cause some tests to fail. Refs: nodejs#48576

kvakil mentioned this pull request Jul 6, 2023

src: pre-emptively fix tests broken by V8 CL #48671

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deps: V8: cherry-pick cb00db4dba6c #48576

deps: V8: cherry-pick cb00db4dba6c #48576

RaisinTen commented Jun 27, 2023

This comment was marked as outdated.

This comment was marked as outdated.

RaisinTen Jun 27, 2023

kvakil Jun 28, 2023

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

kvakil left a comment

kvakil Jun 28, 2023

kvakil commented Jun 28, 2023

RaisinTen commented Jul 3, 2023

targos commented Jul 3, 2023

RaisinTen commented Jul 3, 2023

joyeecheung commented Jul 3, 2023 •

edited

Loading

richardlau commented Jul 3, 2023

RaisinTen commented Jul 6, 2023

deps: V8: cherry-pick cb00db4dba6c #48576

deps: V8: cherry-pick cb00db4dba6c #48576

Conversation

RaisinTen commented Jun 27, 2023

This comment was marked as outdated.

This comment was marked as outdated.

RaisinTen Jun 27, 2023

Choose a reason for hiding this comment

kvakil Jun 28, 2023

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

kvakil left a comment

Choose a reason for hiding this comment

kvakil Jun 28, 2023

Choose a reason for hiding this comment

kvakil commented Jun 28, 2023

RaisinTen commented Jul 3, 2023

targos commented Jul 3, 2023

RaisinTen commented Jul 3, 2023

joyeecheung commented Jul 3, 2023 • edited Loading

richardlau commented Jul 3, 2023

RaisinTen commented Jul 6, 2023

joyeecheung commented Jul 3, 2023 •

edited

Loading