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.

Sign up for GitHub

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

Jump to bottom

[wasm] More jiterpreter cleanup #78519

Merged
merged 24 commits into from
Nov 23, 2022
Merged

[wasm] More jiterpreter cleanup #78519

merged 24 commits into from
Nov 23, 2022

Conversation

kg
Copy link
Contributor

@kg kg commented Nov 17, 2022 •
edited
Loading

Blocks #78428, addresses some PR feedback and quality issues:

  • Move more jiterpreter configuration and constants into options
  • Fix should_generate_trace_here not scanning across multiple basic blocks
  • Disable specialized JIT call in threaded wasm mode (though I think it might work, it's better to turn it off to be sure for now)
  • Introduces genmintops.py, a script that automatically generates mintops.ts from mintops.def
  • Adjust typescript config to make it able to find the generated mintops.ts (and fix ESLint on Linux)
  • Unroll memsets below a certain size into raw wasm opcodes, because v8 generates expensive function calls for memset and memcpy. Unrolling memcpy is a TODO for later
  • Rename "always generate" to "disable heuristic" to more accurately describe what it does
  • Fix jiterpreter_dump_stats hiding errors if startup failed before cwraps were ready
  • Misc. code cleanup

@kg kg added the arch-wasm WebAssembly architecture label Nov 17, 2022
@ghost ghost assigned kg Nov 17, 2022
@ghost
Copy link

ghost commented Nov 17, 2022

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Blocks #78428, addresses some PR feedback and quality issues

Author: kg
Assignees: -
Labels:

arch-wasm
Milestone: -

@build-analysis build-analysis bot mentioned this pull request Nov 18, 2022
Closed
@lambdageek
Copy link
Member

@kg please cherrypick lambdageek@91d2bc9

That will reconfigure the build to:

  1. Run genmintops.py from wasm.proj instead of cmake. This is necessary because cmake runs after rollup, but rollup needs the .ts file.
  2. Place the generated mintops.ts file into artifacts/bin/native/generated

There's a bunch of required rollup and tsconfig changes in there that work around some kind of bug where rollup fails to delegate to typescript when a certain combination of options is used.

As far as I can tell VS Code is happy with these changes - after a build, chasing references in VS Code works - it will find the opcode definitions in the artifacts folder and warn appropriately.

kg and others added 15 commits November 21, 2022 16:36
@kg
Fix problems with the jiterpreter heuristic's basic block traversal
b1f3245 
Remove duplicated minimum opcode counter in the jiterpreter typescript
Move minimum jiterpreter trace hit count to options-def.h
@kg
Move more jiterp configuration into options, clean up a syntax warning
462e4a8
@kg
Fix eslint on linux
abb00e9
@kg
Automatically generate the typescript version of mintops.def at build…
30a047c 
… time using a python script
@kg
Checkpoint raytracer cleanups
de629fe
@kg
Checkpoint mintops ts generation changes
33fbe17
@kg
Unroll small memsets because v8's codegen for memory.fill is awful
03db926
@kg
Checkpoint: Move genmintops
f6b4d3a
@kg
Checkpoint
9446dff
@kg
Move dumpTraces into options.h. Don't update mintops.ts unless the co…
23588ba 
…ntents need to change.
@kg
Fix error being swallowed if a failure happens during startup
5c604bc
@kg
Address PR feedback
be1004b
@lambdageek @kg
generate mintops.ts to artifacts/bin/native/generated
2194785 
- Drive the generation from wasm.proj (driving it from cmake is wrong,
because the cmake build runs after rollup)
- Use the tsconfig rootDirs option to specify where to find the
files (runtime dir and also artifacts/bin/native/generated)
- Also due to rollup issues, specify rootDirs and include in
rollup.config.js - otherwise the rollup typescript plugin doesn't seem
to kick in, and barebones rollup gets confused when parsing .ts files
- The outDir option in tsconfig.json had to be deleted to make rollup
work.  But it was also unused since rollup controls where the ts
plugin is supposed to emit its output
- VS Code understands the tsconfig.json changes and can follow
references to the artifacts/.../generated folder

Note that Release and Debug builds both dump the generates mintops.ts
in the same folder.  This seems ok for now since there's no special
debug-only opcodes.  (Also VS Code has no idea about configurations,
so it has to be done this way or else it won't be able to find the .ts file)
@kg
Checkpoint
cfad6a4
@kg
Repair merge damage
236e4e2
@kg kg force-pushed the wasm-jiterpreter-cleanup-2 branch from fe95daa to 236e4e2 Compare November 22, 2022 00:38
@kg kg marked this pull request as ready for review November 22, 2022 03:35
lambdageek
lambdageek reviewed Nov 22, 2022
View reviewed changes
Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

Some MSBuild nits; and an explicit dependency on mintops.ts for the main rollup target

src/mono/wasm/wasm.proj Outdated Show resolved Hide resolved
src/mono/wasm/wasm.proj Outdated Show resolved Hide resolved
@vargaz
Copy link
Contributor

vargaz commented Nov 22, 2022

The mono changes look ok to me.

@pavelsavara
Copy link
Member 

2. Place the generated `mintops.ts` file into `artifacts/bin/native/generated`

I don't agree that it should be missing in git.
It breaks intelisense until you re-compile.

Perhaps I don't understand all of it.
Why do you think it's good idea @lambdageek ?

@lambdageek
Copy link
Member 

2. Place the generated `mintops.ts` file into `artifacts/bin/native/generated`

I don't agree that it should be missing in git.
It breaks intelisense until you re-compile.

Perhaps I don't understand all of it.
Why do you think it's good idea @lambdageek ?

Checking in a generated file makes workflow more complicated:

  1. For the inner dev loop it's harder to get incremental builds right.
  2. For someone working in the interpreter in another platform it's not obvious that they need to regenerate a wasm file.

As for intellisense, in vs code I only get errors without rebuilding if I open jiterpreter.ts. opening other files works fine.

@kg kg merged commit 6a047a9 into dotnet:main Nov 23, 2022
@build-analysis build-analysis bot mentioned this pull request Nov 23, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vargaz vargaz vargaz left review comments

@lambdageek lambdageek lambdageek left review comments

@BrzVlad BrzVlad BrzVlad left review comments

@pavelsavara pavelsavara pavelsavara approved these changes

@lewing lewing Awaiting requested review from lewing lewing is a code owner

Assignees

@kg kg

Labels
arch-wasm WebAssembly architecture area-Codegen-Interpreter-mono
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kg @lambdageek @vargaz @pavelsavara @BrzVlad