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

Separate lifetime Parser and WootheeResult #6

Merged
merged 3 commits into from
Feb 3, 2018

Conversation

hhatto
Copy link
Member

@hhatto hhatto commented Feb 2, 2018

for #5

@pjenvey
What about this change?

FYI: Performance will decrease slightly.

$ cargo benchcmp bench.old bench.new
 name              bench.old ns/iter  bench.new ns/iter  diff ns/iter  diff %  speedup
 bench_stabilizer  33                 34                            1   3.03%   x 0.97
 smoke_woothee     10,698             11,753                    1,055   9.86%   x 0.91

@hhatto hhatto merged commit 855a53b into master Feb 3, 2018
@hhatto hhatto deleted the separate-lifetime-parser-and-result branch February 3, 2018 04:21
@pjenvey
Copy link
Contributor

pjenvey commented Feb 8, 2018

@hhatto thanks for addressing this. However I'm not sure you need to convert everything to heap allocated strings to solve this. Granted, I haven't looked at every bit of the parser code, but AFAICT what were the original &str's were all 'static, or possibly the lifetime of the original agent &str.

I think the various parser methods may just need more careful handling of lifetimes, in particular, passing around the lifetime of the original agent: &str and tying it to WootheeResult's lifetime (whereas the 'static &strs can always be coereced to this shorter lifetime)

pjenvey added a commit to pjenvey/woothee-rust that referenced this pull request Apr 4, 2018

Verified

This commit was signed with the committer’s verified signature.
pjenvey Philip Jenvey
don't forcefully tie them and clarify lookup_dataset works w/ 'static
WootheeResults

Closes woothee#6
pjenvey added a commit to pjenvey/woothee-rust that referenced this pull request Apr 4, 2018

Verified

This commit was signed with the committer’s verified signature.
pjenvey Philip Jenvey
don't forcefully tie them and clarify lookup_dataset works w/ 'static
WootheeResults

Closes woothee#6
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

2 participants