rhaiscript / rhai

Rhai - An embedded scripting language for Rust.
https://crates.io/crates/rhai
Apache License 2.0
3.77k stars 178 forks source link

Undocumented breaking change in v1.17.0 #828

Closed therealprof closed 3 months ago

therealprof commented 8 months ago

Hi,

after bumping the rhai version on one of our crates, I noticed that there's an undocumented breaking API change here:

https://github.com/rhaiscript/rhai/pull/792/files#diff-15c660271c7e34c78e6b9f1c61305a7fcce209766894eb48d5b9b612cb52438fR180

resulting in:

   --> notifications-handlers/src/bin/rhai-handler.rs:563:35
    |
563 |     const SCOPE: Scope<'static> = Scope::new();
    |                                   ^^^^^^^^^^^^
    |
    = note: calls in constants are limited to constant functions, tuple structs and tuple variants
schungx commented 8 months ago

Thanks for catching this. Weren't expecting a const scope being used anywhere, so Scope::new was changed from const to non-const to accommodate some internal data changes.

Is this critical for you? Can you simply change the offending line?

therealprof commented 8 months ago

Yes, I can. I only need to figure out what the least costly option is to address this.

schungx commented 8 months ago

Since an empty constant scope is essentially equivalent to Scope::new(), you can replace all usage of SCOPE with Scope::new(), which might be better down the road.

Scope changes to use ThinVec which does not have a const constructor... one should be added though. I'll raise an issue.

therealprof commented 8 months ago

Well, functionally that is equivalent. Architecturally the Scope::new() would now be bang in the middle in the execution path of a hot function.

therealprof commented 8 months ago

Luckily there doesn't seem to be any performance impact from using Scope::new() in the hot path.

schungx commented 8 months ago

It is a pity that ThinVec::new is not const... they should be able to make it so.

But I believe using ThinVec is probably a better idea as it shrinks the size of Scope without significant performance impacts.

Cerber-Ursi commented 6 months ago

Luckily there doesn't seem to be any performance impact from using Scope::new() in the hot path.

That's expected, I guess? consts are essentially inlined into every usage place, so from the language's point of view, you had that Scope::new at the same place the whole time.

schungx commented 6 months ago

Well const data structures can simply be bit-copied, while Scope::new would require running a function. Even when inlined, that's still way more instructions than just copying bits.

Cerber-Ursi commented 6 months ago

const items are not bit-copied (semantically - their creation can be optimized to bitwise copy, but so can be Scope::new itself). Using const as a value is exactly the same as using its definition at that place. That's what I was trying to say.

schungx commented 3 months ago

Closing this for now. Feel free to reopen if there are further issues.