search

browsermt / bergamot-translator

Cross platform C++ library focusing on optimized machine translation on the consumer-grade device.
http://browser.mt
Mozilla Public License 2.0
340 stars 38 forks source link

Deprecate unnecessary WebAssembly functions in Response #350

Open jerinphilip opened 2 years ago

jerinphilip commented 2 years ago

The C++ API uses Response as data. source and target are symmetric in structure and this has benefits.

Python bindings almost entirely rely on pybind11 metaprogramming and composition to obtain an easy to maintain, binding friendly value-type.

https://github.com/browsermt/bergamot-translator/blob/a94725b20da9e2f3b1f5303c462d2c6e129c92f9/bindings/python/bergamot.cpp#L166-L170

It is also easier from a maintenance and development perspective that the accessors be consistent across bindings exported.

With a JSON counterpart also coming for Native Messaging (https://github.com/XapaJIaMnu/translateLocally/pull/93), it is important to make the access consistent across Python, C++ and JavaScript, which the developers use.

WebAssembly is also, just bindings. However, WebAssembly appears to planted methods some of which are dead code as of now, creating another means to access the same thing.

https://github.com/browsermt/bergamot-translator/blob/a94725b20da9e2f3b1f5303c462d2c6e129c92f9/src/translator/response.h#L65-L75

https://github.com/browsermt/bergamot-translator/blob/a94725b20da9e2f3b1f5303c462d2c6e129c92f9/wasm/bindings/response_bindings.cpp#L25-L33

This issue proposes to deprecate the WebAssembly added extra functions and constructing bindings via composition to make access to source/target identical and consistent with usages elsewhere.

The changes proposed in this issue will break the WebAssembly test page, but can be fixed quite easily. Also some stuff at https://github.com/mozilla/firefox-translations/blob/6892ad64164787adf1e284141cbd0d10e71fb40d/extension/controller/translation/translationWorker.js at some point, but assuming worker draws from test-page it's solved already by fixing the test-page. It is easier to incur the cost now than later and have all exports of at least Response evolve together.

I am not aware of performance hits we will incur with the convenience mentioned above, but a consistent API expecting the platform (emscripten/WebAssembly) to fix the performance hits in future is what I propose we should still go for.

jerinphilip commented 2 years ago

Replacing .function with .property providing only a getter (no setter) is a potential way to go - https://emscripten.org/docs/api_reference/bind.h.html#_CPPv4NK6class_8propertyEPKcM9ClassType9FieldType.

Experimenting with this for alignments at https://github.com/jerinphilip/bergamot-translator/commit/60e88b62a5939d45ed549077013316af70f00c86.