Improve the debugger script cache#1958
Conversation
kenzieschmoll
left a comment
kenzieschmoll
left a comment
There was a problem hiding this comment.
LGTM with a couple nits
| } | ||
|
|
||
| Future<void> _pause(bool pause) async { | ||
| _isPaused.value = pause; |
There was a problem hiding this comment.
is there a reason setting the notifier was moved inside of the if and else statements respectively? Not a huge deal, but it turns what can be written as one line of code into 2.
There was a problem hiding this comment.
No really; this made it slightly more clear what the two branches do, but I don't think it's critical to do it this way.
| if (_scripts[scriptRef.id] == null) { | ||
| _scripts[scriptRef.id] = scriptRef; | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: not your fault, but I don't think these new lines add much value. Let's remove them to make this method more dense
There was a problem hiding this comment.
Agreed, thinking about this, it's not likely that we'll have gotten a Script object without having already gone through this caching code; removed.
|
cc @grouma for FYI that we're now taking advantage of responsiveness improvements of pausing faster w/ just using the topFrame info. |
2 participants
ScriptCache) instead of using a map in the DebuggerController class, now that the cache has gotten more complicatedpauseEvent.topFramefield, and display the pause immediately, while asynchronously populating the information about the other frames in the background. This will likely reduce the stepping latency a bit when debugging dart web apps (its currently expensive for package:dwds calculate the frame info)Previously, when the script cache was empty, we were sending n requests for scripts when we paused, where n was the number of frames. I.e., the cache didn't handle multiple frames that referenced the same script, where that script was not yet populated in the cache. This issue was found using the new logging code in package:dwds - dart-lang/webdev#1011.