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.

Sign up for GitHub

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

Jump to bottom

Speed up file path resolution when processing stacktraces #603

Merged
merged 1 commit into from Jul 15, 2020
Merged

Speed up file path resolution when processing stacktraces #603

merged 1 commit into from Jul 15, 2020

Conversation

imjoehaines
Copy link
Member

@imjoehaines imjoehaines commented Jul 14, 2020 •
edited

Goal

Following on from #602, this PR removes the Pathname.relative? check entirely

The old Pathname.realpath method was a lot slower so (assuming there was a mix of relative & absolute paths) it was quicker to skip it if possible

With File.realpath, however, it's quick enough that removing the if entirely is faster

With absolute paths (average over 40 runs of 100,000 iterations)

Pathname.relative? & Pathname.realpath Pathname.relative? & File.realpath File.absolute_path? & File.realpath File.realpath only
3.057224275 2.95975725 2.663405825 2.6026659

With relative paths (average over 40 runs of 100,000 iterations)

Pathname.relative? & Pathname.realpath Pathname.relative? & File.realpath File.absolute_path? & File.realpath File.realpath only
4.007721425 3.897996725 3.295810925 3.245656725

Tests

Existing tests ensure nothing has regressed. There's one small change where paths that are absolute but have directory traversal in will now be expanded — I think this isn't possible in a stacktrace anyway, but it's what we'd want to happen anyway

Linked issues

Related to #481

Review

For the submitter, initial self-review:

  • Commented on code changes inline explain the reasoning behind the approach
  • Reviewed the test cases added for completeness and possible points for discussion
  • A changelog entry was added for the goal of this pull request
  • Check the scope of the changeset - is everything in the diff required for the pull request?
  • This pull request is ready for:
    • Initial review of the intended approach, not yet feature complete
    • Structural review of the classes, functions, and properties modified
    • Final review

For the pull request reviewer(s), this changeset has been reviewed for:

  • Consistency across platforms for structures or concepts added or modified
  • Consistency between the changeset and the goal stated above
  • Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
  • Usage friction - is the proposed change in usage cumbersome or complicated?
  • Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
  • Concurrency concerns - if components are accessed asynchronously, what issues will arise
  • Thoroughness of added tests and any missing edge cases
  • Idiomatic use of the language

@imjoehaines
Remove the check for a relative path
f14f39a 
This isn't a _huge_ perf boost, but every little helps :^)

In the old Pathname method, this if was useful because it was quicker
to skip files that were already resolved, assuming there was a mix of
relative and absolute paths

With File.realpath, however, it's quicker to just run the function as
it does nothing if the path is already resolved. This is roughtly
equal in terms of performance to using the new File.absolute_path?
method (they are so close in benchmarks that it's essentially noise)
@imjoehaines imjoehaines changed the title Use File.absolute_path over Pathname.relative Speed up file path resolution when processing stacktraces Jul 15, 2020
@imjoehaines imjoehaines marked this pull request as ready for review July 15, 2020 10:38
@imjoehaines imjoehaines merged commit 2915ced into next Jul 15, 2020
@imjoehaines imjoehaines deleted the use-file-absolute branch July 15, 2020 14:05
@imjoehaines imjoehaines mentioned this pull request Jul 20, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kattrali kattrali kattrali approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@imjoehaines @kattrali