Move filename out of Location

[zherczeg]

Locations should only contain the line info / binary offset, since the filename is the same everywhere. This patch reduces the memory consumption of locations without loosing any information.

[sbc100]

Seems reasonable, but does it really save much memory? (I don't think memory consumption should be a top priority of wabt BTW)

[sbc100]

What does this new boolean field do? Why would you ever not want to print the filename?

[zherczeg]

This is a very interesting question. This particular constructor sets the filename to an empty string:
https://github.com/WebAssembly/wabt/blob/main/include/wabt/common.h#L220

Several tests has no filename info:
https://github.com/WebAssembly/wabt/blob/main/test/binary/bad-data-drop-no-data-count.txt

For me it feels random.

[zherczeg]

The new patch do this without introducing any new members. The ReadBinary function simply uses an empty string as filename when an error is created.

[zherczeg]

It looks like it breaks the emscripten abi. This will not work this way.

[zherczeg]

This is the steps which might lead to a crash:

• Parse the input
• Free the filename string
• Validate the module with an error
• Print error

Because the Location has a reference to the original filename, we likely get a crash. This could be a security error.

It looks like the emscripten abi prevents any changes on this. wabt_validate_module only gets the module, features, errors. The wabt_new_errors has no argument.

Is there any way to fix this? Storing the filename in the module as an std::string is an option.

[zherczeg]

I have reworked the patch. The filename is moved to module (and script), and stored only once, instead of storing for every location. The Error array also keeps the filename reference, so its size is unchanged. This way the patch keeps the same ABI as before, and also reduces the memory consumption (unless the module is empty, but I think that is a very rare case).

As for future works:

• We could add filename info to binary reader. It requires a lot of test rebasing.
• We could use std::string for errors (should be a low number of errors) and modules/scripts. This could prevent possible crashes. I would not say this is high priority, since library users don't complain so far.

[sbc100]

Why is it any better to keep the string_view in the Error object rather than the Location object.

Logically it seems like a Location should always refer to given file... so this doesn't seems to improve things in that sense.

Perhaps you should detail in the PR description how/when this approach is better?

[sbc100]

This seems like an improvement.

[sbc100]

Are there orders of magnitude more Location object constructed than Error objects? I guess that might be a reason to prefer it this way? But also a string_view is only two pointers in size right?

[zherczeg]

I have updated the description. Errors are frequent in a test system, but real world modules are nearly always valid, so no error objects are created most of the time. Locations are part of nearly all expressions / constructs in WebAssembly, since the validator needs them for throwing errors. This patch makes locations "relative" from "absolute", but it should not be a problem in practice. It is true that a string view is only two pointers, but nothing prevents that the target buffer is not freed. The filename-s can be turned to std::string in a follow up patch, which could prevent freeing buffers.

[zherczeg]

For example, every Var in IR has a location member:
https://github.com/WebAssembly/wabt/blob/main/include/wabt/ir.h#L96

[sbc100]

I have updated the description. Errors are frequent in a test system, but real world modules are nearly always valid, so no error objects are created most of the time. Locations are part of nearly all expressions / constructs in WebAssembly, since the validator needs them for throwing errors. This patch makes locations "relative" from "absolute", but it should not be a problem in practice. It is true that a string view is only two pointers, but nothing prevents that the target buffer is not freed. The filename-s can be turned to std::string in a follow up patch, which could prevent freeing buffers.

I think using string_view seems pretty reasonable. The lifetime on the Locations and Errors within a file should not exceed the lifetime of the string holding the filename. We have ASAN/etc to help us be sure of that, right?

If this change is just about move the string_view from the Location to the Error in order to save memory it might be nice to justify with some numbers. How much memory are going to be saving for a real-world-sized module?

[zherczeg]

The filename is not moved from location to error, the error always had a location and that included the filename. Now it is explicit, instead of implicit. I will try to measure memory.

[zherczeg]

I have processed a 35MByte module with wasm2wat.
Original: Peak: 150.0Mb
New: Peak: 132.5Mb
I think this is a nice progress.

Biggest consumers:
ProgramMain: wasm2wat.cc:109: Peak: 34.9Mb
OnGenericCustomSection: src/binary-reader-ir.cc:1888: 33.2Mb
These are present in both, and not even relevant from our perspective.

The next entries are more interesting.
BinaryReaderIR::OnLocalGetExpr: src/binary-reader-ir.cc:1077 Peak: 23.8Mb -> 18.2Mb
BinaryReaderIR::OnI32ConstExpr: src/binary-reader-ir.cc:1082 Peak: 11.8Mb -> 9.2Mb
BinaryReaderIR::OnLoadExpr: src/binary-reader-ir.cc:1102 Peak: 6.4Mb -> 5.1Mb
BinaryReaderIR::OnStoreExpr: src/binary-reader-ir.cc:1236 Peak: 5.4Mb -> 4.3Mb
And so on

[zherczeg]

Thank you!