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

c-writer.cc: Add local symbol prefix. #2171

Merged
merged 3 commits into from
Mar 15, 2023
Merged

c-writer.cc: Add local symbol prefix. #2171

merged 3 commits into from
Mar 15, 2023

Conversation

yhdengh
Copy link
Contributor

@yhdengh yhdengh commented Mar 15, 2023 •
edited
Loading

This adds kLocalSymbolPrefix which is used for names of params, locals and stack vars. This allows c-writer to not assign global_syms_ to local_syms_ for writing each individual function, since local names can't duplicate global names.

This speeds up the execution of wasm2c on large inputs. clang.wasm is a 75M WASM module. Before this PR, wasm2c on clang.wasm would take ~63 minutes.

time ./build/wasm2c clang.wasm -o clang.c --enable-multi-memory
real    63m42.780s
user    63m15.818s
sys     0m24.107s

After this PR, it takes ~2 minutes.

time ./build/wasm2c clang.wasm -o clang.c --enable-multi-memory
real    2m12.889s
user    2m8.853s
sys     0m3.983s

@yhdengh
c-writer.cc: Add local symbol prefix.
2838078 
Add kLocalSymbolPrefix which is used for names of params, locals and
stack vars. This allows c-writer to not assign global_sym_map_ to
local_sym_map_ for writing each individual function, since local names
can't duplicate global names.
sbc100
sbc100 reviewed Mar 15, 2023
View reviewed changes
Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Wow! Impressive speedup.

Do you know where the huge speedup comes from? Was is just making N copies of the global symbol map?

src/c-writer.cc Outdated
@@ -2360,7 +2361,7 @@ void CWriter::PushFuncSection(std::string_view include_condition) {
void CWriter::Write(const Func& func) {
func_ = &func;
// Copy symbols from global symbol table so we don't shadow them.
Copy link
Member

Choose a reason for hiding this comment

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

I guess this comment is not longer accurate?

I guess its not possible for a global symbol could start with var_?

Copy link
Contributor Author

@yhdengh yhdengh Mar 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Global symbols should either start with w2c_ or wasm2c_, and therefore local symbols would not shadow global symbols. Deleted this comment in ce882d6.

sbc100
sbc100 reviewed Mar 15, 2023
View reviewed changes
src/c-writer.cc Outdated
@@ -428,6 +428,7 @@ static constexpr char kParamSuffix =
static constexpr char kLabelSuffix = kParamSuffix + 1;

static constexpr char kSymbolPrefix[] = "w2c_";
Copy link
Member

Choose a reason for hiding this comment

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

Should we rename this kGlobalSymbolPrefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in ce882d6.

sbc100
sbc100 reviewed Mar 15, 2023
View reviewed changes
src/c-writer.cc
@@ -428,6 +428,7 @@ static constexpr char kParamSuffix =
static constexpr char kLabelSuffix = kParamSuffix + 1;

static constexpr char kSymbolPrefix[] = "w2c_";
static constexpr char kLocalSymbolPrefix[] = "var_";
Copy link
Member

Choose a reason for hiding this comment

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

I guess unlike the other prefixes, this one is 100% internal so changing it is not a user-visible change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. So should I change kLocalSymbolPrefix to be a private field of CWriter?

Copy link
Member

Choose a reason for hiding this comment

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

So sorry, I just meant that if we decided to change var_ to local_ in the figure it would not be user visible?

This change lgtm.

@yhdengh
Copy link
Contributor Author

yhdengh commented Mar 15, 2023 •
edited
Loading

Based on our perf report, when executing wasm2c on clang.wasm, 90% of the time was spent on assigning global_syms_ to local_syms_. So, yes, this huge speedup comes from not making N copies of global_syms_. But how much this PR speeds up wasm2c would, I guess, differ from module to module, since each module has a different size of global_syms_.

sbc100
sbc100 approved these changes Mar 15, 2023
View reviewed changes
Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm but maybe @keithw wants to take a look too.

Those performance numbers are hard to argue with though! Nice work.

keithw
keithw approved these changes Mar 15, 2023
View reviewed changes
Copy link
Member

@keithw keithw left a comment

Choose a reason for hiding this comment

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

looks great to me % comment

src/c-writer.cc Outdated
@@ -830,9 +831,9 @@ std::string CWriter::DefineGlobalScopeName(ModuleFieldType type,
/* Names for params, locals, and stack vars are formatted as "w2c_" + name. */
Copy link
Member

Choose a reason for hiding this comment

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

Let's update this comment too...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in f4427d2

Copy link
Member

Choose a reason for hiding this comment

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

Great, setting to auto-merge.

@keithw keithw enabled auto-merge (squash) March 15, 2023 01:48
@keithw keithw mentioned this pull request Mar 15, 2023
Open
12 tasks
@keithw keithw merged commit 905f614 into WebAssembly:main Mar 15, 2023
@yhdengh yhdengh deleted the remove-set-assign branch March 15, 2023 20:22
@keithw keithw mentioned this pull request Sep 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keithw keithw keithw approved these changes

@sbc100 sbc100 sbc100 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yhdengh @keithw @sbc100