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

Allow datrie with recent Python / Drop datrie? #442

Closed
gsnedders opened this issue Feb 26, 2020 · 7 comments
Closed

Allow datrie with recent Python / Drop datrie? #442

gsnedders opened this issue Feb 26, 2020 · 7 comments

Comments

@gsnedders
Copy link
Member

@gsnedders gsnedders commented Feb 26, 2020

We limit the datrie optional dependency to Python < 3.7, because prior to the 0.8 release it didn't support Python 3.7. Probably, we should either:

  • Require datrie >= 0.8
  • Drop support for datrie (this depends in part of the perf impact of doing so)
@hugovk
Copy link
Contributor

@hugovk hugovk commented Feb 26, 2020

Please see PR #446 to require datrie 0.8+ to support Python 3.7.

But datrie doesn't yet support Python 3.8: pytries/datrie#73.

@gsnedders
Copy link
Member Author

@gsnedders gsnedders commented Feb 26, 2020

But it doesn't yet support Python 3.8: pytries/datrie#73

Ergh, yeah, okay, should just take a look at the perf impact of just dropping support.

@gsnedders
Copy link
Member Author

@gsnedders gsnedders commented Feb 29, 2020

Ergh, yeah, okay, should just take a look at the perf impact of just dropping support.

In a bunch of more ordinary cases the perf impact seems to be approximately nothing. I suggest we just drop support for datrie? I doubt even in the worst case it's going to be significant?

@hugovk
Copy link
Contributor

@hugovk hugovk commented Feb 29, 2020

If keeping it only offers minimal or zero performance benefit, sounds like a good idea to drop, it'll reduce the maintenance burden, as it seems this will happen for every new (now annual) Python release. And we're 4 months since 3.8 came out, and Datrie support is still pending.

@tacaswell
Copy link

@tacaswell tacaswell commented Mar 26, 2020

datries 0.8.2 should support py37 and py38.

@gsnedders
Copy link
Member Author

@gsnedders gsnedders commented May 19, 2020

On the two tests I have that are pretty heavy on entities, we get:

Mean +- std dev: [datrie_without] 293 ms +- 6 ms -> [datrie_with] 291 ms +- 6 ms: 1.01x faster (-1%)
Mean +- std dev: [datrie_without] 104 ms +- 1 ms -> [datrie_with] 104 ms +- 1 ms: 1.01x faster (-1%)

On the whole, despite both tests showing statistically significant perf gains, I think they're small enough they're not worth it?

It's probably worthwhile noting the pure-Python entity lookup with was much slower when datrie was added as an optional dependency, and as such the perf gain from it was much larger.

@gsnedders
Copy link
Member Author

@gsnedders gsnedders commented Jun 10, 2020

#455 dropped datrie.

@gsnedders gsnedders closed this Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.