Exposing JsValue::ZERO and ONE constants

[Ekleog]

Hey!

I'm writing bindings for IndexedDB, and it turns out IndexedDB does not support indexing a boolean column. The recommended way to do it is to basically index a 0/1 column, or even a column where absence of the key indicates false.

As a consequence, using these bindings means people have to use JsValue::from(1) and JsValue::from(0) quite a lot. IIUC, it means one heap allocation over FFI each time. One alternative way to do it would be to use a lazy-static, but it also feels like quite a lot of effort for things that should be reasonably simple — zero and one are pretty often-used constants.

WDYT about adding JsValue::ZERO and JsValue::ONE constants? I tried looking into making a PR for it right away, but then noticed there's some RESERVED index that seems to have some significant, so I thought I'd ask about your thoughts here first :)

(And yes this is most certainly premature optimization considering there's a disk access going on anyway, and I'll live without any issue if this feature does not land, it's just that making heap allocations like this make me sad inside)

Anyway, as usual, thank you for all you do, it's a pleasure using the rust wasm ecosystem ❤️

[daxpedda]

I wouldn't mind that, but it should probably live on js_sys::Number instead if that's possible.

Just to make sure, did you try JsValue::FALSE and JsValue::TRUE?
Maybe some implicit conversion (not sure about JS semantics here) would do the trick.

[RReverser]

This feels like a slippery slope TBH, given that everyone will have their own ideas about most useful constants for different types. Usually thread_local! is the recommended way for interned JsValues, and I'd rather keep recommending that.

[Ekleog]

@daxpedda The reason why I didn't suggest putting these on js_sys::Number is because of the already-existing constants on Number, that are Rust values and not JsValues — mixing the two would sound like a bad idea to me. (Also, I cannot use JsValue::FALSE/TRUE because they're not valid IndexedDB keys, and there's no autocast happening before that)

@RReverser I see the point, but OTOH I don't think there's many more values that would make sense without any domain-specific knowledge. And thread-locals are not the most pleasant thing to use when it's just for very simple values — something like the js-intern crate would seem more usable to me. I personally just do the heap allocation for now, and believe that most likely others do the same because of the hassle of interning manually.

If I were to push the slippery slope to its end, JsValue would end up with the following constants:

• FALSE, TRUE, NULL and UNDEFINED that are already present
• ZERO, ONE
• EMPTY_STRING, EMPTY_ARRAY and EMPTY_OBJECT

I cannot think of anything more that would make sense as an officially-interned value. Putting them all would pass the number of pre-interned values from 4 to 9.

WDYT?

[RReverser]

• ZERO, ONE

MINUS_ONE? How about all the constants already in js_sys::Number like NAN, POSITIVE_INFINITY, NEGATIVE_INFINITY, etc. but also as JsValues instead of Rust-side f64s? Those definitely come up often.

Personally, when something like this comes up and I want to specialise bult-ins for e.g. performance, I tent to just declare my own #[wasm_bindgen] imports.

Then you have full control and can declare extra methods with the same js_name but accepting e.g. u32 instead of JsValue, which avoids the allocation, avoids hardcoding constants, and actually would be even cheaper than sharing JsValue since JS doesn't need to look up the value by index - it just gets the primitive number right away from Wasm.

• EMPTY_ARRAY and EMPTY_OBJECT

Note that those would be particularly problematic when passing to APIs that might accidentally modify them, as it would mean sharing the same object by reference.

[Ekleog]

MINUS_ONE? How about all the constants already in js_sys::Number like NAN, POSITIVE_INFINITY, NEGATIVE_INFINITY, etc. but also as JsValues instead of Rust-side f64s? Those definitely come up often.

Do they come up as JsValues? I definitely see the point in having them as f64, but I'm surprised that you'd need them as JsValues. That said I might be missing something :)

For MINUS_ONE, I also had it in the first version of my message above, and then noticed that I don't think any widely-used JS API actually would have a special-case for MINUS_ONE? But yes, this one was the first in my list of "let's not add them even at the end of the slippery slope"

Note that those would be particularly problematic when passing to APIs that might accidentally modify them, as it would mean sharing the same object by reference.

That's a good point! Argument against them, so they're not even in the slippery slope, and that brings us down to 7 (or 8 with NEGATIVE_ONE) constants on JsValue at the end of the slippery slope.

Personally, when something like this comes up and I want to specialise bult-ins for e.g. performance, I tent to just declare my own #[wasm_bindgen] imports.

This is definitely a good solution for someone who knows how to use wasm-bindgen and has high performance requirements 😅

I still think that the most widely used constants could deserve having a hardcoded value, because end-users don't want to bother defining their own imports when it's just to build one live array. To give an example, the use case that triggered this discussion is:

idb_index.get(&Array::from_iter([&JsValue::from(1), &object_id_js]))

Where each use of the various indexes is slightly different in shape.

That said, I won't press this issue further. I personally think that 8 constants is an OK number for JsValue, but I also see your point, and will let you make the final decision here :) (If you were to decide in favor of it, I'll still be happy to write a PR that implements it)

Thank you for the chat!

[RReverser]

but I also see your point, and will let you make the final decision here :)

Oh, please don't. I probably should have clarified it sooner, but while I contributed to wasm-bindgen and built other tooling around it, I'm not a maintainer of this repo. I just had an opinion that I felt I need to voice :)

[daxpedda]

@daxpedda The reason why I didn't suggest putting these on js_sys::Number is because of the already-existing constants on Number, that are Rust values and not JsValues — mixing the two would sound like a bad idea to me.

Adding them to JsValue seems to have the same issue though: you would have to name them NUMBER_ONE, otherwise it could be anything, e.g. a BigInt. Also looking at JsValue::TRUE it's applied to JsValue and not to js_sys::Boolean, so I think you are right, we should stick with adding them to JsValue.

This feels like a slippery slope TBH, given that everyone will have their own ideas about most useful constants for different types.

I really don't mind adding ZERO and ONE, but I'm unsure if it makes sense for this use-case in the first place because most of our APIs take f64/i32/u32 directly. Only few APIs that take array<any> have this problem.

Considering this is about a performance optimization, I agree with @RReverser, you should probably add some custom bindings. Maybe it would make more sense to add Array::push_f64(), which would more adequately solve the issue then JsValue::ZERO/ONE.

---

I still really don't mind adding JsValue::ZERO/ONE, so I would accept a PR. But @Ekleog I don't really think this appropriately addresses your issue: you are much better served by adding some custom bindings.

[RReverser]

Maybe it would make more sense to add Array::push_f64(), which would more adequately solve the issue then JsValue::ZERO/ONE.

Another thing that would really help here is the long-awaited tuple support (#122, whether via native multivalue or plain JS), as then one could do idb_index.get(&JsValue::from((1, &object_id_js))).

[Ekleog]

Thank you for all your answers and the discussion! I'm going to follow #122 pretty closely, it looks quite interesting.

TBH I don't think I have any particular performance issue: my code is called just before/after reaching out to disk anyway, so it's probably not too big a deal to make one more allocation. This was mostly about a potential avenue for improvements for wasm-bindgen itself, because using from all the time just feels iffy.

I'm going to close this, because the arguments against are currently basically as strong as the arguments in favor. If anyone else comes here with the same need please feel free to raise the issue back up from the dead! An user who actually cares a lot about this might be all that's needed to push it through :)