emscripten-core / emscripten

Emscripten: An LLVM-to-WebAssembly Compiler
Other
25.75k stars 3.3k forks source link

embind should use the length that std::string already knows #13500

Open hsivonen opened 3 years ago

hsivonen commented 3 years ago

Per a comment on another issue embind doesn't make use of std::string knowing its length when converting to a JS string. Treating std::string as a C string is both unnecessarily inefficient and, in the case of embedded U+0000, in correct.

Please use the length already known and avoid strlen when converting a std::string to a JS string.

juj commented 3 years ago

So... in https://github.com/whatwg/encoding/issues/172 we asked for a browser spec to help implement performant support for marshalling C strings because we pointed out that TextDecoder only supports C++ strings, needing to strlen the strings in advance is a performance issue. The feedback from you was that doing the extra strlen is not a compelling enough performance problem to warrant adding a new API, and that there were no real world applications being impacted by the problem.

Now you point out that embind doing that very same strlen is unnecessarily inefficient and that it should be optimized. 🤷 Would not have expected that.

I wish that unnecessary inefficiency could also be addressed for web application developers dealing with C strings.

Sarcasm aside, the case of supporting embedding zeroes in a std::string is a valid point indeed.

hsivonen commented 3 years ago

I still think running strlen in Wasm vs. running strlen on the browser side is not compelling enough a case to add new Web API surface.

However, when you have something that already knows the string length, like std::string does, recomputing the length is needlessly inefficient regardless of where it's done.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

hsivonen commented 2 years ago

AFAICT, this is still a valid issue.