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

Proof of concept bindings for fetch_delete_key and a regression test. #658

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nwc10
Copy link
Contributor

@nwc10 nwc10 commented Aug 22, 2020

@lizmat - IIRC you said that a big annoyance with hashes was that to implement the Raku semantics on the API exposed by NQP, the code has to

  1. fetch the value for the key
  2. then delete the key
  3. so that it can return the value

and quite often the value isn't even needed, but the implementation doesn't know that at the point that it's fetching it.

So, here's a proof of concept NQP op that can fetch and delete in one. I can do this bit easily. I can't do the "adapt Rakudo to use it".

Could you test using this in Rakudo to see how much it improves the relevant performance?

I didn't really want to bother @jnthn (or even $jnthn) with the "other" issues unless we know that it's something worth doing.

As best I can tell, other issues are

  1. yet more C code in MoarVM, and I can't see an easy way to cleanly de-duplicate it. C doesn't have templates.
  2. this name doesn't seem to fit the MoarVM op naming convention - what should it be?
  3. this code is sort-of-bypassing the repr, so it's a bit of a hack (but it might be the leat hacky approach)
  4. I implemented it by mostly copying atkey_o (so I don't know if I failed to spot something)
  5. I did spot that deletekey has a serialisation barrier, so I added that, but I don't know what I missed
  6. The JIT "implementation" is a blatant copy of atkey_o and might not even work. I think that I'm testing it, but I'm not certain.

Apart from all that, the test passes, and I know that it hits the relevant code - you'll need MoarVM/fetch-and-delete which is a branch off AAAA-A-better-hash. (I'll rebase it onto master once we merge tomorrow-ish)

@nwc10 nwc10 requested a review from lizmat August 22, 2020 07:51
@coke coke changed the base branch from master to main April 19, 2023 13:36
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.

None yet

1 participant