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

Attempt FALLBACK_SET/GET on failure to find field via SET/GET #440

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

LoopyAshy
Copy link

@LoopyAshy LoopyAshy commented Oct 19, 2022

I made it possible to fallback to Protocol::INDEX_GETProtocol::FALLBACK_GET with StaticString/String key on failure to find a field via Protocol::GET, the same applies to Protocol::INDEX_SETProtocol::FALLBACK_SET and Protocol::SET.

Reasoning

Sometimes it is desired for user readability to pretend a specific custom type has fields and instead accessing the data from elsewhere such as from a HashMap or another collection.
possible use cases is having types with dynamic layouts while having rules to their usage such as locking the layout once created.
it also allows custom types to be dynamically extended by the scripter while keeping the benefits of being a static Shared<AnyObj> but not seeming like a collection which [] accesses give the impression of.

Benchmarks

Note
Due to the tiny footprints of the functions, the differences are very hard to see if any and can be improved with a faster hashmap regardless but honestly values these small are easily within margin of error.

Before

test fields::actual_field_get                    ... bench:         418 ns/iter (+/- 88)
test fields::actual_field_set                    ... bench:         434 ns/iter (+/- 161)

test fields::string_key_classic_index_get        ... bench:         523 ns/iter (+/- 52)
test fields::static_string_key_classic_index_get ... bench:         486 ns/iter (+/- 80)

After

test fields::actual_field_get                     ... bench:         413 ns/iter (+/- 47)
test fields::actual_field_set                     ... bench:         432 ns/iter (+/- 25)

test fields::string_key_classic_index_get         ... bench:         500 ns/iter (+/- 14)
test fields::static_string_key_classic_index_get  ... bench:         469 ns/iter (+/- 32)

test fields::string_key_fallback_field_get        ... bench:         499 ns/iter (+/- 15)
test fields::static_string_key_fallback_field_get ... bench:         469 ns/iter (+/- 20)

test fields::string_fallback_field_set            ... bench:         596 ns/iter (+/- 24)
test fields::static_string_fallback_field_set     ... bench:         608 ns/iter (+/- 14)

Closes

#381 #263

Notes

Regarding the changes done to this PR, I do wonder if FALLBACK_* is preferred choice compared to META_* or META_FIELD_* since in most script languages I have used, META usually implies it does it first/instead not after, which although being a option would be a fine choice.

I do like the advantage of the current design being that it prioritises actual fields first.

I do wonder if it was changed to use the 'meta' getter/setter first if it would be plausible to pass in the original field getter and setter functions so u could do this:

|this: &Custom, default_get: NativeFunction, name: String| if let Ok(v) = default_get(name) {
    Ok(v)
} else {
    //Do my field get
}

This approach however would have overhead and although could be faster for types which only want 'meta' fields I definitely feel it would cost for classic field get/sets which would be a waste and a shame and it isn't the most attractive solution regardless.

Another possible solution could be explicitly declaring a type as having 'meta' fields and which order to prioritise finding them such as MetaFieldAccess::First MetaFieldAccess::Last, MetaFieldAccess::Always and finally MetaFieldAccess::Never(default) which would be drastically cheaper and simpler than it being passed in as a argument to the meta function which in my opinion reduces the understandability for implementors.
The enum system could even be applied to the Any derive macro as an attribute to make it easy to implement and recognise by looking at types.

Made it possible to fallback to Protocol::INDEX_GET on failure to find a field via Protocol::GET, same applies to Protocol::INDEX_SET and Protocol::SET
Copy link
Collaborator

@udoprog udoprog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

Apart the use of unsafe: I'm also hesitant to have two protocols bleed together like this. What prevents you from implementing both of them separately (GET and INDEX_GET) on the type you want both operations to work with?

crates/rune/src/runtime/vm.rs Outdated Show resolved Hide resolved
@LoopyAshy LoopyAshy requested a review from udoprog October 19, 2022 15:40
Separated fallback into its own protocol.
Renamed fallback protocols to be consistent with INDEX_GET and INDEX_SET.
@LoopyAshy LoopyAshy changed the title Attempt INDEX_SET/GET on failure to find field via SET/GET Attempt FALLBACK_SET/GET on failure to find field via SET/GET Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants