James Long (:jlongster) Reporter
	Description • 10 years ago

Eddy, I know we were going to meet about this today, but I thought I'd go ahead and start this discussion. Let's all meet next week.

As far as I can tell, almost all (maybe even all) our uses of synchronize right now are centered around waiting until a sourcemap is available. We need this because we need sourcemaps in the debugger hooks, but the hooks are synchronous and the sourcemap may not be downloaded and parsed yet.

The problem is that this is horrible for performance. We are blocking the entire UI to wait for a sourcemap. This really impacts the perceived usability of our tools, and I have several real anecdotes about this: https://twitter.com/dan_abramov/status/591763823423524864

"Let me know, I like to give FF a spin every now and then but the source pane beachballs too much for me" - @dan_abromov who is a really great developer

I know fitzgen believes that the RDP should handle sourcemaps transparently, meaning that any location coming from the RDP should already be sourcemapped. The frontend doesn't really know anything about sourcemaps.

But this leads directly to this behavior; there's no other way to not block the entire UI while a sourcemap is parsed, which can be quite a long time because sourcemaps are terrible.

What would we do for the profiler? Are we really going to sourcemap *all* of those locations before even giving it to the UI to start rendering? That would really bad usability-wise.

Sourcemaps are an "after the fact" thing. We shouldn't sacrifice so much for them.

Here's what Chrome does, and what I think we need to do: sourcemap a location on the server *if* a sourcemap is readily available (and we already have access to the location; the profiler might need to parse a big object first so it still might not even do it). Otherwise, go ahead and notify the client that something happened. This will depend on the action: for the console logs, just go ahead and send out a raw location, and then later the server can send the sourcemapped location and the console will update it. For the debugger, it could be switched into the paused state but will be waiting for frames & sources.

The point is: when any of our debugger hooks are fired or a console.log happens, immediately notify the UI no matter what, and parse the sourcemap in a web worker. Then, when it's available, follow-up with the client to do whatever needs to be done.

This should not affect the semantics of almost all our hooks. There are few places where we will lost correctness, like setting breakpoints immediately when the page loads (see bug 1163854). Not blocking in onNewScript could set breakpoints in wrong locations, but honestly it will affect such a small amount of cases that I don't think we need to worry about it.

I filed bug 1163798 for parsing sourcemaps in a webworker, but that's not going to help at all until we fix this bug.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 1 • 9 years ago

(In reply to James Long (:jlongster) from comment #0)
> Eddy, I know we were going to meet about this today, but I thought I'd go
> ahead and start this discussion. Let's all meet next week.
> 
> As far as I can tell, almost all (maybe even all) our uses of synchronize
> right now are centered around waiting until a sourcemap is available. We
> need this because we need sourcemaps in the debugger hooks, but the hooks
> are synchronous and the sourcemap may not be downloaded and parsed yet.

How do we reset breakpoints without a source map? How do you avoid race conditions involving new sources and source mapped BPs?

How do we tell if a source is blackboxed or not (and whether we should pause or continue) without knowing the original source?

What can we do besides wait for the source map in these cases? Both gdb and lldb need to load debug symbols before you can debug your C/C++, how is this any different?

> The problem is that this is horrible for performance. We are blocking the
> entire UI to wait for a sourcemap.

In which case are we blocking the UI? All I can think of is loading the initial source list, but even then the UI should still be responsive since it is in a different process.

Do you just mean displaying some things that aren't affected by source mapping eagerly? Is this in fact a better UI? Is it worth the additional complexity? Maybe in some cases, I'm not sure.

I think a good thing to do would be to emit RDP events at the start and end of synchronous source map fetching and parsing, and then the frontend could use those to have a "loading and parsing debug symbols" message/spinner.

> This really impacts the perceived
> usability of our tools, and I have several real anecdotes about this:
> https://twitter.com/dan_abramov/status/591763823423524864
> 
> "Let me know, I like to give FF a spin every now and then but the source
> pane beachballs too much for me" - @dan_abromov who is a really great
> developer

You were the one who mentioned source maps as a possible reason for being slow in that thread. We don't know what it is for sure, and that isn't evidence of people complaining about this specific problem. It could be that we are spending time elsewhere. We need a test case to investigate further.

> I know fitzgen believes that the RDP should handle sourcemaps transparently,
> meaning that any location coming from the RDP should already be
> sourcemapped. The frontend doesn't really know anything about sourcemaps.

Well, the backend HAS to know about source maps to decide whether to pause in a lot of places. The (separate) decision we made when implementing source maps was for the RDP to be opaque about whether a given source is source mapped or not.

I wouldn't be against something like Safari's source mapping UI where the source mapped sources are children of the generated code in the source tree. This would involve sending both source mapped and non-source mapped locations and sources over the RDP. But this doesn't change the fact that you would still need the source map to reset BPs or check blackboxing, etc.

> But this leads directly to this behavior; there's no other way to not block
> the entire UI while a sourcemap is parsed, which can be quite a long time
> because sourcemaps are terrible.

No, it is the fact that we don't know where to set BPs or whether the source is black boxed without the source map that leads to this. FWIW, I doubt that synchronously getting+parsing source maps to check blackboxing while stepping is an issue (we should already have the source map by then). I suspect the synchronous getting/parsing of source maps is only an issue with newly introduced sources and resetting BPs.

> 
> What would we do for the profiler? Are we really going to sourcemap *all* of
> those locations before even giving it to the UI to start rendering? That
> would really bad usability-wise.
>

The profiler is different in that we don't need to potentially halt execution of JS at a given place/time because of what info the source map does or does not give us.

The profiler and source mapping is a completely different conversation. As is the console and source mapping, etc.
 
> Here's what Chrome does, and what I think we need to do: sourcemap a
> location on the server *if* a sourcemap is readily available (and we already
> have access to the location; the profiler might need to parse a big object
> first so it still might not even do it). Otherwise, go ahead and notify the
> client that something happened. This will depend on the action: for the
> console logs, just go ahead and send out a raw location, and then later the
> server can send the sourcemapped location and the console will update it.

This works for anything that doesn't need to make a decision based on the source mapped info, such as profiler or console.

> For the debugger, it could be switched into the paused state but will be
> waiting for frames & sources.

This would introduce race conditions for source mapped sources, breakpoints, and reload.

> 
> The point is: when any of our debugger hooks are fired or a console.log
> happens, immediately notify the UI no matter what, and parse the sourcemap
> in a web worker. Then, when it's available, follow-up with the client to do
> whatever needs to be done.

Worker is only needed for non-e10s, but I guess it would be nice to only have one code path.

Again, this works for the console and profiler, but not for resetting BPs in the debugger, nor for determining whether we want to pause or ignore and continue due to blackboxing or not. Doesn't work anytime we need to make a decision based on info in the source map.

> This should not affect the semantics of almost all our hooks. There are few
> places where we will lost correctness, like setting breakpoints immediately
> when the page loads (see bug 1163854). Not blocking in onNewScript could set
> breakpoints in wrong locations, but honestly it will affect such a small
> amount of cases that I don't think we need to worry about it.

I'm of the opposite opinion. People did file bugs and did complain when these race conditions used to exist. We must be correct; we are a debugger! If we can't exhibit correct behavior, how can devs use or trust our tools to verify/fix their apps' behavior?! We *MUST* present the actual behavior of their program's logic, meaning that there aren't race conditions that make it look like some function isn't called when in fact it is.

	James Long (:jlongster) Reporter
	Comment 2 • 9 years ago

I'll respond more thoroughly tomorrow, I need to leave now, but wanted to reply to this:

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #1)
> > This really impacts the perceived
> > usability of our tools, and I have several real anecdotes about this:
> > https://twitter.com/dan_abramov/status/591763823423524864
> > 
> > "Let me know, I like to give FF a spin every now and then but the source
> > pane beachballs too much for me" - @dan_abromov who is a really great
> > developer
> 
> You were the one who mentioned source maps as a possible reason for being
> slow in that thread. We don't know what it is for sure, and that isn't
> evidence of people complaining about this specific problem. It could be that
> we are spending time elsewhere. We need a test case to investigate further.

It's most definitely sourcemaps. I worked with him a little further and he turned off sourcemaps and everything was smooth. And it makes total sense too: we completely block the work to parse sourcemaps, and large apps that use webpack generate a huge sourcemap, so it takes a while to parse.

I think there's huge evidence that sacrificing a tiny amount of correctness has big wins for usability, since Chrome sacrifices this but people rave about their sourcemap support.

Sorry for the quick reply, I'll reply more tomorrow.

	James Long (:jlongster) Reporter
	Comment 3 • 9 years ago

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #1)
> (In reply to James Long (:jlongster) from comment #0)
> > Eddy, I know we were going to meet about this today, but I thought I'd go
> > ahead and start this discussion. Let's all meet next week.
> > 
> > As far as I can tell, almost all (maybe even all) our uses of synchronize
> > right now are centered around waiting until a sourcemap is available. We
> > need this because we need sourcemaps in the debugger hooks, but the hooks
> > are synchronous and the sourcemap may not be downloaded and parsed yet.
> 
> How do we reset breakpoints without a source map? How do you avoid race
> conditions involving new sources and source mapped BPs?
> 
> How do we tell if a source is blackboxed or not (and whether we should pause
> or continue) without knowing the original source?

You referenced this below, but the only time we have a problem is when the page is loaded. That initial few seconds that the sourcemap is being parsed. Whatever happens in this period is where we can lose some correctness, but I haven't found that to be problematic.

We store generated locations like we did before, that what you do with breakpoints.

At load time, we can't tell if the original source is blackboxed or not.

All of these things lose some correctness, yes, but in real-world usage I think you hardly ever hit them. I say this because I've talked about it with *several* developers that absolutely *love* Chrome's sourcemap support, even if they say "yeah, once in a while I need to manually move the breakpoint because the soucemap changed".

The cost of trying to achieve perfect leads to a serious issue that makes our tools feel janky at load time. And remember that in debugging you frequently reload your page. When the cost of reloading the page is so severe it just makes the whole thing feel slow.

I know it's sourcemap parsing making the debugger load slow because I asked a developer with a huge site (meaning huge sourcemap) to load the tools with sourcemaps on and off. When they are on, the UI beachballs for a few seconds at load time, after that it is smooth.

I can do some more performance testing to verify this further.

> 
> What can we do besides wait for the source map in these cases? Both gdb and
> lldb need to load debug symbols before you can debug your C/C++, how is this
> any different?

It's different because sourcemaps suck and they take forever to parse. We can just act on the info we have, which only affects a few things at load time.

> 
> > The problem is that this is horrible for performance. We are blocking the
> > entire UI to wait for a sourcemap.
> 
> In which case are we blocking the UI? All I can think of is loading the
> initial source list, but even then the UI should still be responsive since
> it is in a different process.
> 
> Do you just mean displaying some things that aren't affected by source
> mapping eagerly? Is this in fact a better UI? Is it worth the additional
> complexity? Maybe in some cases, I'm not sure.
> 
> I think a good thing to do would be to emit RDP events at the start and end
> of synchronous source map fetching and parsing, and then the frontend could
> use those to have a "loading and parsing debug symbols" message/spinner.

Now that I think about e10s, I'm not sure about the beachballing. That may help. I will look into our performance more seriously and get back to you. I think what's happening is that `synchronize` blocks the page, which makes Firefox beachball, even if our tools are in a different process. I'm a little confused now though, I'll do more research.

This also has implications like bug 1163854.

I mean displaying what we can to the UI. If it's the debugger, yeah, it just means pause and show a message that we loading the source contents. More importantly, in the console, print the message with the generated line number, but when a sourcemap is available update the line number.

Think about the console: we can't possibly block until the sourcemap is ready to start showing messages. Now the several-second-blocking-on-load problem will affect our entire devtools.

> 
> > This really impacts the perceived
> > usability of our tools, and I have several real anecdotes about this:
> > https://twitter.com/dan_abramov/status/591763823423524864
> > 
> > "Let me know, I like to give FF a spin every now and then but the source
> > pane beachballs too much for me" - @dan_abromov who is a really great
> > developer
> 
> You were the one who mentioned source maps as a possible reason for being
> slow in that thread. We don't know what it is for sure, and that isn't
> evidence of people complaining about this specific problem. It could be that
> we are spending time elsewhere. We need a test case to investigate further.

As I've said elsewhere, it's definitely something with sourcemaps. He turned sourcemaps off and it's very smooth, and it's smooth once the sources are loaded. There's a several second pause when the tools are loaded with sourcemaps enabled.

> 
> > I know fitzgen believes that the RDP should handle sourcemaps transparently,
> > meaning that any location coming from the RDP should already be
> > sourcemapped. The frontend doesn't really know anything about sourcemaps.
> 
> Well, the backend HAS to know about source maps to decide whether to pause
> in a lot of places. The (separate) decision we made when implementing source
> maps was for the RDP to be opaque about whether a given source is source
> mapped or not.
>
> 
> I wouldn't be against something like Safari's source mapping UI where the
> source mapped sources are children of the generated code in the source tree.
> This would involve sending both source mapped and non-source mapped
> locations and sources over the RDP. But this doesn't change the fact that
> you would still need the source map to reset BPs or check blackboxing, etc.

Yeah, I think we'll end up doing something like that when we have a tree view.

I'm saying that the times that we need to decide to pause but the sourcemaps aren't available are only at load time, and it's ok to act as if there isn't a sourcemap then.

> 
> > But this leads directly to this behavior; there's no other way to not block
> > the entire UI while a sourcemap is parsed, which can be quite a long time
> > because sourcemaps are terrible.
> 
> No, it is the fact that we don't know where to set BPs or whether the source
> is black boxed without the source map that leads to this. FWIW, I doubt that
> synchronously getting+parsing source maps to check blackboxing while
> stepping is an issue (we should already have the source map by then). I
> suspect the synchronous getting/parsing of source maps is only an issue with
> newly introduced sources and resetting BPs.
> 

Yep, it's only at load. As I've said above: let's store generated locations instead so we know where to set them at load time. Requiring  sourcemaps at load time *does* lead directly to this blocking behavior. We can sacrifice some correctness to make our tools feel a *lot* faster.

> > 
> > What would we do for the profiler? Are we really going to sourcemap *all* of
> > those locations before even giving it to the UI to start rendering? That
> > would really bad usability-wise.
> >
> 
> The profiler is different in that we don't need to potentially halt
> execution of JS at a given place/time because of what info the source map
> does or does not give us.
> 
> The profiler and source mapping is a completely different conversation. As
> is the console and source mapping, etc.

They are not a different conversation. I don't know much about the profiler. But for the console, we absolutely need to sourcemap the line numbers there. And to do that we need access to the sourcemap. Right now the pattern is the wait until the sourcemap is available, but we can't do that for the console.

If you're saying the console could update locations async, while the debugger blocks, well, that could work. You'd still see a slowdown in the debugger though.

>  
> > Here's what Chrome does, and what I think we need to do: sourcemap a
> > location on the server *if* a sourcemap is readily available (and we already
> > have access to the location; the profiler might need to parse a big object
> > first so it still might not even do it). Otherwise, go ahead and notify the
> > client that something happened. This will depend on the action: for the
> > console logs, just go ahead and send out a raw location, and then later the
> > server can send the sourcemapped location and the console will update it.
> 
> This works for anything that doesn't need to make a decision based on the
> source mapped info, such as profiler or console.

As I've said, these decisions can lose a little bit of accuracy and developers don't seem to care.

> 
> > For the debugger, it could be switched into the paused state but will be
> > waiting for frames & sources.
> 
> This would introduce race conditions for source mapped sources, breakpoints,
> and reload.

I don't know what you mean by race conditions. It would introduce a few inaccuracies when the sourcemaps has changed by a different file. But developers don't seem to care.

> 
> > 
> > The point is: when any of our debugger hooks are fired or a console.log
> > happens, immediately notify the UI no matter what, and parse the sourcemap
> > in a web worker. Then, when it's available, follow-up with the client to do
> > whatever needs to be done.
> 
> Worker is only needed for non-e10s, but I guess it would be nice to only
> have one code path.
> 
> Again, this works for the console and profiler, but not for resetting BPs in
> the debugger, nor for determining whether we want to pause or ignore and
> continue due to blackboxing or not. Doesn't work anytime we need to make a
> decision based on info in the source map.

See all my points above. It is possible.

> 
> > This should not affect the semantics of almost all our hooks. There are few
> > places where we will lost correctness, like setting breakpoints immediately
> > when the page loads (see bug 1163854). Not blocking in onNewScript could set
> > breakpoints in wrong locations, but honestly it will affect such a small
> > amount of cases that I don't think we need to worry about it.
> 
> I'm of the opposite opinion. People did file bugs and did complain when
> these race conditions used to exist. We must be correct; we are a debugger!
> If we can't exhibit correct behavior, how can devs use or trust our tools to
> verify/fix their apps' behavior?! We *MUST* present the actual behavior of
> their program's logic, meaning that there aren't race conditions that make
> it look like some function isn't called when in fact it is.

I'm not what race conditions you mean. Whatever bugs we had before, all I know is that we have to at least match Chrome in this case. And worrying about perfection is hurting us more then helping, imho, and we still can't sourcemap anything other than the debugger.

I looked at the chrome devtools source code and found something interesting: they actually do all the sourcemap parsing on the client-side, so the server doesn't even know about it. It looks like they also load in more information about the scripts though, so more of there breakpoint logic is on the client. Not saying this a better way to do it at all though.

	James Long (:jlongster) Reporter
	Comment 4 • 9 years ago

Case in point: go to http://stampsy.com/about, open up the debugger, set a breakpoint, and reload the page. It's pretty bad. This is a great place to start profiling and see what's going on. I suspect it is blocking on sourcemaps in _addScript.

	James Long (:jlongster) Reporter
	Comment 5 • 9 years ago

I did some preliminary profiling, and something is weird. I may be wrong.

I timed sourcemap creation and parsing on stampsy.com. It takes 37ms to create the SourceMapConsumer (mostly JSON.parse I'd guess) and ~800ms to parse the actual mappings if a breakpoint is set.

Something else is killing performance though. When a breakpoint is set it feel like takes forever for the page to reload, maybe 5-10 seconds.

I think e10s has helped us, so it's not beach-balling anymore, and the devtools UI is not blocked. But the page *is* blocked for that period of time.

I don't know what's blocking though if it's not parsing the sourcemap mappings.

For the debugger, we might be able to wait until the sourcemap is loaded to do anything if it only takes ~800ms in an advanced real-world scenario. That's less than I thought. Still, I'd like to argue that we don't even have to wait that long.

But it's not really worth diving into that discussion until we figure out what the heck is taking it so long to load.

We also still need to fix bug 1163854, so we need to figure out a "quick" fix for it if we are not going to remove synchronize (for now).

	James Long (:jlongster) Reporter
	Comment 6 • 9 years ago

Here's some hacky code that records some timings and plays around with parsing the sourcemap in a webworker. I'm not really sure what the benefit of parsing it in a web worker is if we are always going to use `synchronize` to block on it anyway.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 7 • 9 years ago

(In reply to James Long (:jlongster) from comment #6)
> Created attachment 8607764 [details]
> sourcemap-worker.js
> 
> Here's some hacky code that records some timings and plays around with
> parsing the sourcemap in a webworker. I'm not really sure what the benefit
> of parsing it in a web worker is if we are always going to use `synchronize`
> to block on it anyway.

Well we could spawn N workers, where N=number of cores, and then just synchronize on all of them finishing. This would help if there are multiple source maps that need to be parsed.

	James Long (:jlongster) Reporter
	Comment 8 • 9 years ago

oops, meant to make this a patch before