Refactor breakpoints system #405

vspinu · 2017-05-30T22:44:51Z

This addresses clojure-emacs/cider#1699. The core of the change is to remove most of the global dynamic vars and rely on lexical binding on top of the main instrumented form for all the common metadata. Currently same metadata is replicated again and again for each sub-form. Another major change is to reduce the macros machinery to two basic function calls break and apply-instrumented-maybe.

This pretty much reaches theoretical minimum for the size of generated clojure code, one call per instrumented form. Pushing this further would require understanding lower level details of what and how eats into the java's 64KB method limit space.

The following code

(let [i 1]
  (+ i
     (* i i)))

now expands into:

(let*
 [META_
  {:code "#dbg\n(let [i 1]\n  (+ i\n     (* i i)))\n",
   :original-id "12",
   :file "/home/vspinu/tmp/tmp.clj",
   :line 10,
   :column 0}]
 (cider.nrepl.middleware.debug/break
  []
  (let*
   [i 1]
   (cider.nrepl.middleware.debug/break
    [2]
    (+
     (cider.nrepl.middleware.debug/break [2 1] i META_ {})
     (cider.nrepl.middleware.debug/break
      [2 2]
      (*
       (cider.nrepl.middleware.debug/break [2 2 1] i META_ {})
       (cider.nrepl.middleware.debug/break [2 2 2] i META_ {}))
      META_
      {}))
    META_
    {}))
  META_
  {}))

instead of the current:

(let*
 []
 (clojure.core/push-thread-bindings
  (clojure.core/hash-map
   #'cider.nrepl.middleware.debug/*skip-breaks*
   (let*
    [or__4238__auto__ cider.nrepl.middleware.debug/*skip-breaks*]
    (if or__4238__auto__ or__4238__auto__ (clojure.core/atom nil)))
   #'cider.nrepl.middleware.debug/*locals*
   {}
   #'cider.nrepl.middleware.debug/*extras*
   {:code "#dbg\n(let [i 1]\n  (+ i\n     (* i i)))\n",
    :original-id "20",
    :file "/home/vspinu/tmp/tmp.clj",
    :line 20,
    :column 0,
    :coor []}
   #'cider.nrepl.middleware.debug/*pprint-fn*
   (:pprint-fn
    clojure.tools.nrepl.middleware.interruptible-eval/*msg*)))
 (try
  (if
   (clojure.core/not
    (clojure.core/seq @cider.nrepl.middleware.debug/debugger-message))
   (do (throw (new java.lang.Exception "Debugger not initialized"))))
  (let*
   [val__6618__auto__
    (let*
     [i 1]
     (let*
      []
      (clojure.core/push-thread-bindings
       (clojure.core/hash-map
        #'cider.nrepl.middleware.debug/*skip-breaks*
        (let*
         
[or__4238__auto__ cider.nrepl.middleware.debug/*skip-breaks*]
         (if
          or__4238__auto__
          or__4238__auto__
          (clojure.core/atom nil)))
        #'cider.nrepl.middleware.debug/*locals*
        {}
        #'cider.nrepl.middleware.debug/*extras*
        {:code "#dbg\n(let [i 1]\n  (+ i\n     (* i i)))\n",
         :original-id "20",
         :file "/home/vspinu/tmp/tmp.clj",
         :line 20,
         :column 0,
         :coor [2]}
        #'cider.nrepl.middleware.debug/*pprint-fn*
        (:pprint-fn
         clojure.tools.nrepl.middleware.interruptible-eval/*msg*)))

.............................................................................
.............................................................................
clip 282 lines
.............................................................................
.............................................................................

   (if
    (cider.nrepl.middleware.debug/skip-breaks? [])
    val__6618__auto__
    (if
     (clojure.core/=
      (:mode @cider.nrepl.middleware.debug/*skip-breaks*)
      :trace)
     (do
      (cider.nrepl.middleware.debug/print-step-indented
       0
       '(let* [i 1] (+ i (* i i)))
       val__6618__auto__)
      val__6618__auto__)
     (if
      :else
      (cider.nrepl.middleware.debug/read-debug-command
       val__6618__auto__
       (clojure.core/assoc
        cider.nrepl.middleware.debug/*extras*

        :debug-value
        (cider.nrepl.middleware.debug/pr-short val__6618__auto__)))
      nil))))
  (finally (clojure.core/pop-thread-bindings))))

bbatsov · 2017-05-31T04:52:41Z

@vspinu Impressive work!

@Malabarba please, take a look at the changes.

bbatsov · 2017-05-31T05:06:00Z

src/cider/nrepl/middleware/util/instrument.clj

@@ -253,6 +253,15 @@
  ;; Don't use `postwalk` because it destroys previous metadata.
  (walk-indexed #(tag-form %2 breakfunction) form))

+(defn print-form [form & [expand? meta?]]


You can add some docstring here.

bbatsov · 2017-05-31T05:07:29Z

src/cider/nrepl/middleware/util/instrument.clj

@@ -137,8 +135,7 @@
   (let [{extras ::extras,
          [_ orig] ::original-form,
          bf   ::breakfunction} (meta form)]
-     (when verbose-debug
-       (println "[DBG]" (not (not bf)) extras (or orig form)))
+     ;; (println "[DBG]" (not (not bf)) extras (or orig form))


I won't leave the commented out code here.

expez · 2017-05-31T09:44:23Z

test/clj/cider/nrepl/middleware/debug_test.clj

-      ;; Top-level sexps are not debugged, just returned.
-      (is (= (eval `(let [~'x 10] (d/breakpoint d/*locals* {:coor []} nil)))
-             '{x 10})))))
+;; (deftest breakpoint


Leaving this commented out was probably not intended

Right. Good catch.

expez · 2017-05-31T09:46:22Z

Thank you so much for digging into this @vspinu!

I hit this all the time, but I haven't had to time to do anything about it.

vspinu · 2017-05-31T13:43:31Z

One thing that I am not very clear about is why eval-with-locals uses the dynamic *ppprint-fn* (here). This doesn't seem necessary so I changed it to a more direct (:pprint-fn *msg*).

vspinu · 2017-06-01T01:59:05Z

For the record. Two more factors contribute to the size of the method - original forms that are inlined into the final code and locals (&env expansion). If any of these two is large you can get "method too large" even with a simple #break.

The original forms could be saved in a separate atom with a simple macro trick, but locals really need to be inlined. So I am "fixing" the "locals issue" by simply recompiling without locals when "method code too large!" is caught. You won't be able to use commands that depend on locals (l,e,j,p) but all others will work as expected. It's a good tradeoff.

dpsutton · 2017-06-01T02:01:48Z

do you think it would be possible for you to put together a markdown document giving a general overview of how the debugger works? I'd imagine no one knows more about it that you right now.

Malabarba · 2017-06-01T11:52:39Z

Thanks indeed @vspinu . I'll try to have a look as soon as possible.

vspinu · 2017-06-01T19:11:30Z

a markdown document giving a general overview of how the debugger works?

I am not currently in the position to do that as I don't have a good understanding of the emacs-debuger communication. Some design choices are still not clear to me (for instance why compulsory initialization?, why blocking debugger?). This PR mostly concerns with the breakpoint logic.

Only @Malabarba could make such a write-up but I am not sure it would be worth it at this stage. The debugger is still a moving target and there are already quite a few documentation notes in the code.

grammati · 2017-06-02T15:13:30Z

For what it's worth, I have been trying out this change and it works perfectly, as far as I can tell.

I have tried out all the debug operations, and have not seen any regressions. I have also tried debugging functions that used to fail with "Method code too large", and they can be debugged now. Yay!

👍

Malabarba · 2017-06-04T14:43:21Z

WRT a markdown guide: there's an issue on this repo (or maybe on the cider repo) about supporting cljs in the debugger. On that issue I made a very short guise on how the debugger works.
I could write a more completed one if I ever find the time.

Malabarba · 2017-06-08T00:36:30Z

src/cider/nrepl/middleware/util/misc.clj

+  [a b]
+  (= (seq a) (seq b)))
+
+


Malabarba · 2017-06-08T01:04:43Z

src/cider/nrepl/middleware/util/instrument.clj

+                     (let [br (::breakfunction (meta f))
+                           f1 (m/strip-meta f keys)]
+                       (if br
+                         (vary-meta f1 assoc :instrumented (name (:name (meta br))))


Why use (name (:name (meta br))) instead of just br?

Also, we should use ::instrumented.

Final form has only :instrumented string tag in it. Keeping full function around leads to huge clutter when you print the form with meta and generally makes it much harder to understand what is what. Each meta key also adds 5 extra bytecode ops to the final object size on every breakpoint invocation. After all it's just a tag and should be a string or a symbol, especially when you send it to emacs.

The non-qualified :instruented is intentional. I am not bound to it but it looks to me that qualification is not warranted in this context. The:instrumented tag is more generic than :cider.nrepl.middleware.util.instrument/instrumented. Different middleware or different implementation of instrumentation should be able to use it without sending that sausage around. It also rather annoying in printouts as doesn't bear extra informational content there. On emacs side it feels like spillage of internals as well. See associated changes.

Malabarba · 2017-06-08T01:32:39Z

src/cider/nrepl/middleware/enlighten.clj

-                              (->> (pr-very-short out#)
-                                   (assoc d/*extras* :status :enlighten :debug-value)
-                                   d/debugger-send))
+                            (->> (assoc (:msg_ ~'META_)


Is the _ in :msg_ intentional?

Typo indeed.

Malabarba · 2017-06-08T01:46:49Z

Besides a few comments, I think it looks good. And it must have taken a lot of work. Thanks a lot for the effort.

Malabarba · 2017-06-08T01:48:23Z

Also what's that META_ symbol you are using? Is that some syntax I never heard of?

bbatsov · 2017-06-08T11:27:54Z

@vspinu I'll merge this once you address @Malabarba's comments. And you might also want to combine some of the commits together, but that's not particularly important to me.

vspinu · 2017-06-08T13:35:45Z

Also what's that META_ symbol you are using?

It's just an anaphoric local variable. It needs to be accessed from all breakpoint macros and as clojure autoqualifies symbols in macros you need to inhibit the auto-qualification by quoting with ~' everywhere. I appended _ at the end to signify that it's an internal symbol which is automatically cleaned up in sanitize-env along all clojure temporaries with _ in them. Also to avoid potential conflicts with user variables named META.

BTW, that cleaning of local variables will break if users use locals with _, but that's not really a regression.

vspinu · 2017-06-08T13:45:34Z

cleaning of local variables will break if users use locals with _,

Could be improved by removing symbols with __ only. AFAICS all clojure temporaries use that. So I guess I will also use META__ then.

bbatsov · 2017-06-09T06:13:47Z

Taht could be robustified by removing symbols with __ only. AFAICS all clojure temporaries use that. So I guess I will also use META__ then.

Sounds good. Also add some explanations about this in the code itself.

bbatsov · 2017-06-14T06:20:31Z

@vspinu ping :-)

vspinu · 2017-06-14T09:04:52Z

Yes. I will be back on it this weekend. Got a rather tight two weeks.

bbatsov · 2017-06-18T05:03:46Z

OK. You'll also have to rebase this on top of the current master.

xiongtx · 2017-06-18T05:56:05Z

I don't understand the changes, but the new expansion seems much more pleasing than the old 😄.

- Common metadata is now let bound once at the top of the form. - Macros expand into parsimonious 'break' and `apply-instrumented-maybe` functions. This bring the size of the expanded code to one call per breakpoint. - Only `coor` is hooked in local `extras` metadata - Remove verbose-debug. It's much less useful now as there are more points of interests. It's easier to insert `print-form` whenever needed.

- save original forms within local map - recompile with no &env bindings when "code too large!" method detected

Reasons: - Each meta keywors shows 5 java ops in bytecode - Inspection of code with functions in metadata is a nightmare

- Rename META_ into META__ and document - Only strip local variables with __ in their names - Use :cider/instrumented as instrumentation tag - Document defonces - Move *skip-breaks* reset to wrap-debug to make the intention clearer - Update enlighten to nested META_

- Rename resolve-special -> special-sym-meta - Don't check for special-symbol? explicitly but rely on special-sym-meta instead - Remove unnecessary duplicated computation in info-clj

vspinu · 2017-06-18T21:41:38Z

I am done with addressing all comments and rebasing & squashing it. Added one more "side" commit relating to #410.

One cell in travis matrix fails for non-sider reason.

bbatsov · 2017-06-20T09:06:09Z

👍

bbatsov reviewed May 31, 2017

View reviewed changes

expez reviewed May 31, 2017

View reviewed changes

vspinu mentioned this pull request Jun 1, 2017

Cancelling prompt while debugging quits debugger #339

Closed

vspinu force-pushed the break branch from 2bf0d17 to 8f2b347 Compare June 1, 2017 23:22

vspinu mentioned this pull request Jun 3, 2017

Accompanying changes to clojure-emacs/cider-nrepl#405 clojure-emacs/cider#2002

Merged

vspinu force-pushed the break branch 2 times, most recently from 8430601 to 9773df2 Compare June 3, 2017 21:32

This was referenced Jun 5, 2017

Evaluation in debugger must be done within the namespace of instrumented form clojure-emacs/cider#2004

Closed

Implement some notion of debugging session clojure-emacs/cider#1054

Closed

Malabarba reviewed Jun 8, 2017

View reviewed changes

src/cider/nrepl/middleware/util/misc.clj Outdated

[a b]

(= (seq a) (seq b)))

Copy link

Member

Malabarba Jun 8, 2017

Choose a reason for hiding this comment

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

Whitespace

Malabarba reviewed Jun 8, 2017

View reviewed changes

vspinu added 2 commits June 18, 2017 18:03

Add print-form function

f472c0f

vspinu mentioned this pull request Jun 18, 2017

Include special forms in apropos #410

Merged

5 tasks

vspinu added 6 commits June 18, 2017 20:24

Move var meta extractors from info, apropos and debug to utils.meta

696ac41

Move coord<, random-uuid-str and seq= to more suitable namespaces

71c7589

Two more tricks to solve "method code too large!" issue

22ddb08

- save original forms within local map - recompile with no &env bindings when "code too large!" method detected

Simplify and strip instrumentation meta

b97c125

Reasons: - Each meta keywors shows 5 java ops in bytecode - Inspection of code with functions in metadata is a nightmare

Move debugger-message check to debugger-send

785a618

Don't send to client unavailable debug commands

e967213

vspinu force-pushed the break branch 3 times, most recently from 71cdb34 to 650d8af Compare June 18, 2017 19:35

vspinu added 2 commits June 18, 2017 21:41

Streamline special symbols' meta handling

21e72c1

- Rename resolve-special -> special-sym-meta - Don't check for special-symbol? explicitly but rely on special-sym-meta instead - Remove unnecessary duplicated computation in info-clj

vspinu force-pushed the break branch from 650d8af to 21e72c1 Compare June 18, 2017 19:42

bbatsov merged commit 0406734 into clojure-emacs:master Jun 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor breakpoints system #405

Refactor breakpoints system #405

vspinu commented May 30, 2017 •

edited

Loading

bbatsov commented May 31, 2017

bbatsov May 31, 2017

bbatsov May 31, 2017

expez May 31, 2017

vspinu May 31, 2017

expez commented May 31, 2017

vspinu commented May 31, 2017

vspinu commented Jun 1, 2017 •

edited

Loading

dpsutton commented Jun 1, 2017

Malabarba commented Jun 1, 2017

vspinu commented Jun 1, 2017

grammati commented Jun 2, 2017

Malabarba commented Jun 4, 2017

Malabarba Jun 8, 2017

Malabarba Jun 8, 2017

Malabarba Jun 8, 2017

vspinu Jun 8, 2017

Malabarba Jun 8, 2017

vspinu Jun 8, 2017

Malabarba commented Jun 8, 2017

Malabarba commented Jun 8, 2017

bbatsov commented Jun 8, 2017

vspinu commented Jun 8, 2017 •

edited

Loading

vspinu commented Jun 8, 2017 •

edited

Loading

bbatsov commented Jun 9, 2017

bbatsov commented Jun 14, 2017

vspinu commented Jun 14, 2017

bbatsov commented Jun 18, 2017

xiongtx commented Jun 18, 2017

vspinu commented Jun 18, 2017

bbatsov commented Jun 20, 2017

Refactor breakpoints system #405

Refactor breakpoints system #405

Conversation

vspinu commented May 30, 2017 • edited Loading

bbatsov commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

expez commented May 31, 2017

vspinu commented May 31, 2017

vspinu commented Jun 1, 2017 • edited Loading

dpsutton commented Jun 1, 2017

Malabarba commented Jun 1, 2017

vspinu commented Jun 1, 2017

grammati commented Jun 2, 2017

Malabarba commented Jun 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Malabarba commented Jun 8, 2017

Malabarba commented Jun 8, 2017

bbatsov commented Jun 8, 2017

vspinu commented Jun 8, 2017 • edited Loading

vspinu commented Jun 8, 2017 • edited Loading

bbatsov commented Jun 9, 2017

bbatsov commented Jun 14, 2017

vspinu commented Jun 14, 2017

bbatsov commented Jun 18, 2017

xiongtx commented Jun 18, 2017

vspinu commented Jun 18, 2017

bbatsov commented Jun 20, 2017

vspinu commented May 30, 2017 •

edited

Loading

vspinu commented Jun 1, 2017 •

edited

Loading

vspinu commented Jun 8, 2017 •

edited

Loading

vspinu commented Jun 8, 2017 •

edited

Loading