Skip to content

Conversation

coolreader18
Copy link
Member

No description provided.

@coolreader18 coolreader18 force-pushed the coolreader18/string-hash-field branch from 24dc622 to 0def37d Compare September 19, 2019 22:46
Copy link
Contributor

@palaviv palaviv left a comment

Choose a reason for hiding this comment

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

Looks good. Does it have an improvement in the benchmarks?

host: PyStringRef::try_from_object(vm, tuple.elements[0].clone())?
.value
.to_string(),
host: vm.to_pystr(&tuple.elements[0])?,
Copy link
Contributor

Choose a reason for hiding this comment

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

This actually convert the first argument in the tuple to string without validation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, that's why the tests are failing.

@coolreader18
Copy link
Member Author

@palaviv I'm not sure if it improves variable lookup but it definitely should improve string-keyed dict lookup, given that it's the exact same string object. I think string interning is the next key feature to make this optimization really take effect.

@coolreader18 coolreader18 merged commit 00a0e45 into master Sep 21, 2019
@coolreader18 coolreader18 deleted the coolreader18/string-hash-field branch September 21, 2019 22:15
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