Skip to content

Conversation

oxinabox
Copy link
Owner

#27
There is 1 key internal change of this, that should be invisible to users, more or less.
Rather than create a Method using @which, to decide if a method should be instrumented or not.
we instead query the function + argument against the set of rules.
Which is much faster, expectially in the case where no, or few @no_debug rules are set.
(as in the benchmark below).

However, there is 1 problem with this.
If you set_nodebug on a method, that takes fairly abstract arguments.
Then it will actually result in all more specific methods also being blocked.
I don't think that that is actually a serious usability issue.
As set_nodebug is mostly used ona function or module level.

  • One possible resolution is to just ban set_nodebug from working on a method level.
  • Another is to set make it legal but print a warning explaining its behavour.

A less problematic side effect of this change is that now setting no debug on a Module,
now takes out methods that belong to that module's functions,
even when those methods are defined in other modules.
I do not think this change is a problem, as I think that is the more reasonable behavour to have.
It is about knocking out a whole namepace, not knocking out a particular package.

using the "standard" summer test
for benchmarking debuggers:
(I have a feeling I am not using it the same way as in JuliaDebug/JuliaInterpreter.jl#204 (comment) though)

With this PR:

julia> @btime @iron_debug summer($(ones(40)))
  4.238 ms (16043 allocations: 626.09 KiB)
40.0

julia> @btime @iron_debug summer_inlined($(ones(40)))
  2.312 ms (7956 allocations: 284.27 KiB)
40.0

Current master:

julia> @btime @iron_debug summer($(ones(40)))
  9.792 ms (28160 allocations: 1.29 MiB)
40.0

julia> @btime @iron_debug summer_inlined($(ones(40)))
  3.545 ms (13097 allocations: 641.72 KiB)
40.0

@oxinabox
Copy link
Owner Author

Ok, I went with disabling setting of nodebug on methods.
Not really a useful feature anyway.

Assuming CI passes, then I will probably merge this tomorrow.

@oxinabox oxinabox merged commit 40e13de into master Apr 23, 2019
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.

1 participant