Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Undocumented breaking change in v1.17.0 #828

Open
therealprof opened this issue Jan 31, 2024 · 9 comments
Open

Undocumented breaking change in v1.17.0 #828

therealprof opened this issue Jan 31, 2024 · 9 comments

Comments

@therealprof
Copy link

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
Copy link
Collaborator

schungx commented Jan 31, 2024

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
Copy link
Author

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

@schungx
Copy link
Collaborator

schungx commented Feb 1, 2024

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
Copy link
Author

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
Copy link
Author

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

@schungx
Copy link
Collaborator

schungx commented Feb 1, 2024

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
Copy link

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
Copy link
Collaborator

schungx commented Mar 28, 2024

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
Copy link

Cerber-Ursi commented Mar 28, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants