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

Replace the slow slotname Dict with a counter ("age")-based mechanism #307

Merged
merged 2 commits into from
Aug 16, 2019

Conversation

timholy
Copy link
Member

@timholy timholy commented Jul 27, 2019

Old benchmarks:

          "recursive other 1_000" => Trial(7.510 ms)
          "recursive self 1_000" => Trial(2.450 ms)
          "long function 5_000" => Trial(33.257 ms)
          "throw long 1_000" => Trial(14.002 ms)
          "tight loop 10_000" => Trial(213.216 ms)

vs this branch:

         "recursive other 1_000" => Trial(7.502 ms)
          "recursive self 1_000" => Trial(1.996 ms)
          "long function 5_000" => Trial(30.500 ms)
          "throw long 1_000" => Trial(13.406 ms)
          "tight loop 10_000" => Trial(174.004 ms)

Take with a grain of salt, there is a fair amount of variance. But the last one is consistently cut to about 80% of the original.

@codecov-io
Copy link

codecov-io commented Jul 27, 2019

Codecov Report

Merging #307 into master will increase coverage by 0.3%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #307     +/-   ##
=========================================
+ Coverage   88.17%   88.47%   +0.3%     
=========================================
  Files          11       11             
  Lines        1742     1753     +11     
=========================================
+ Hits         1536     1551     +15     
+ Misses        206      202      -4
Impacted Files Coverage Δ
src/construct.jl 92.18% <100%> (-0.22%) ⬇️
src/types.jl 72.54% <100%> (+1.12%) ⬆️
src/breakpoints.jl 92.64% <100%> (+3.67%) ⬆️
src/interpret.jl 86.31% <100%> (ø) ⬆️
src/utils.jl 85.57% <100%> (+0.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9ebc94...9bb6176. Read the comment docs.

src/types.jl Outdated
@@ -176,10 +180,11 @@ mutable struct Frame
framecode::FrameCode
framedata::FrameData
pc::Int
assignment_counter::Int
Copy link
Member

@KristofferC KristofferC Jul 27, 2019

Choose a reason for hiding this comment

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

Could potentially make this a concrete Int64 if we worry at all about overflow (on 32-bit)

@timholy
Copy link
Member Author

timholy commented Jul 28, 2019

Does the approval mean you know it doesn't break Debugger.jl? How about Juno?

@KristofferC
Copy link
Member

It means the PR on its own is good but it is indeed breaking Debugger. Will fix that up tomorrow.

@KristofferC
Copy link
Member

JuliaDebug/Debugger.jl#198 should fix the Debugger.jl side.

@timholy
Copy link
Member Author

timholy commented Jul 29, 2019

Let's wait for @pfitzseb to get a chance to look it over, in case it has more negatives than we might imagine.

@KristofferC
Copy link
Member

JunoLab/Atom.jl#148 should fix the Juno side.

@KristofferC
Copy link
Member

I think we should just put eval_code into this package.

@pfitzseb
Copy link
Member

Agreed, it feels bad to have to keep both copies in sync...

@KristofferC
Copy link
Member

Ok, with eval_code in this repo and updated in this PR I will merge this.

@KristofferC KristofferC merged commit d4596e3 into master Aug 16, 2019
@KristofferC KristofferC deleted the nodict branch August 16, 2019 12:29
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.

4 participants