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

Implementing break on load #241

Merged
merged 6 commits into from Nov 28, 2017

Conversation

Projects
None yet
4 participants
@rakatyal
Member

rakatyal commented Sep 27, 2017

No description provided.

@auchenberg

This comment has been minimized.

Show comment
Hide comment
@auchenberg

auchenberg Sep 27, 2017

Contributor

Any particular reason for why we don't use DOMDebugger.setInstrumentationBreakpoint as per recommendation from the Chrome team?

Microsoft/vscode-chrome-debug#445

Contributor

auchenberg commented Sep 27, 2017

Any particular reason for why we don't use DOMDebugger.setInstrumentationBreakpoint as per recommendation from the Chrome team?

Microsoft/vscode-chrome-debug#445

@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Sep 27, 2017

Member

Because it has that issue which might never be fixed, and this is blocking chrome-debug in VS.

Member

roblourens commented Sep 27, 2017

Because it has that issue which might never be fixed, and this is blocking chrome-debug in VS.

@auchenberg

This comment has been minimized.

Show comment
Hide comment
@auchenberg

auchenberg Sep 27, 2017

Contributor

@roblourens @rakatyal It's blocking a very particular use case of HTML imports in Chrome, which is a platform issue, so we implement a workaround in our debugger? It's my understanding that break-on-load using the recommended way works for the vast majority of Chrome users?

What's the pro's and con's of our own implementation of this?

Contributor

auchenberg commented Sep 27, 2017

@roblourens @rakatyal It's blocking a very particular use case of HTML imports in Chrome, which is a platform issue, so we implement a workaround in our debugger? It's my understanding that break-on-load using the recommended way works for the vast majority of Chrome users?

What's the pro's and con's of our own implementation of this?

@rakatyal

This comment has been minimized.

Show comment
Hide comment
@rakatyal

rakatyal Sep 27, 2017

Member

@auchenberg: I believe apart from the flag not working in the HTML imports scenario, there are some other reasons to go this way:

  1. We implemented the same logic for break on load to work for Webkit debugger inside VS. So this is a tried and tested approach.
  2. We are not aware of the performance impact of the experimental flag since we never got around to test it out. Plus it's experimental, so we can't be sure of it's implementation in the future.

So we agree that this might not be the best solution out there, but it's a safe bet for us now.
What do you think?

Member

rakatyal commented Sep 27, 2017

@auchenberg: I believe apart from the flag not working in the HTML imports scenario, there are some other reasons to go this way:

  1. We implemented the same logic for break on load to work for Webkit debugger inside VS. So this is a tried and tested approach.
  2. We are not aware of the performance impact of the experimental flag since we never got around to test it out. Plus it's experimental, so we can't be sure of it's implementation in the future.

So we agree that this might not be the best solution out there, but it's a safe bet for us now.
What do you think?

@auchenberg

This comment has been minimized.

Show comment
Hide comment
@auchenberg

auchenberg Sep 27, 2017

Contributor

@rakatyal

I understand that we have prior art from VS, but it's also my understanding that we were using the Chrome recommended API before we encountered the HTML imports issues, correct? We should therefore know the performance characteristics and behavior of the APIs.

I don't understand why we don't want to offload this functionality to the platform (Chrome) by using an API that's exposed for this purpose and already well-used inside Chrome DevTools.

As I see this then this approach is growing our implementation and maintainability surface within our debugger for no apparent gain. That said this is an engineering decision, and if you think this is the best solution now and going forward then that's the way we roll.

Contributor

auchenberg commented Sep 27, 2017

@rakatyal

I understand that we have prior art from VS, but it's also my understanding that we were using the Chrome recommended API before we encountered the HTML imports issues, correct? We should therefore know the performance characteristics and behavior of the APIs.

I don't understand why we don't want to offload this functionality to the platform (Chrome) by using an API that's exposed for this purpose and already well-used inside Chrome DevTools.

As I see this then this approach is growing our implementation and maintainability surface within our debugger for no apparent gain. That said this is an engineering decision, and if you think this is the best solution now and going forward then that's the way we roll.

@rakatyal

This comment has been minimized.

Show comment
Hide comment
@rakatyal

rakatyal Sep 27, 2017

Member

@auchenberg: I agree that ideally we should be offloading this functionality to the platform. But from my understanding we never actually got to using that flag and hence never actually measured the performance impact of it. @digeff should be able to answer this concretely though, but we may think that flag would have had adverse performance impacts when compared to this approach.

Member

rakatyal commented Sep 27, 2017

@auchenberg: I agree that ideally we should be offloading this functionality to the platform. But from my understanding we never actually got to using that flag and hence never actually measured the performance impact of it. @digeff should be able to answer this concretely though, but we may think that flag would have had adverse performance impacts when compared to this approach.

@digeff

This comment has been minimized.

Show comment
Hide comment
@digeff

digeff Sep 28, 2017

Contributor

@auchenberg We never tested the performance of that flag. We did test the performance of using a breakpoint on .*:0:0 to break on the first line of each and every file, which is similar to that command.

When using .*:0:0 in a project with 100s JavaScript file, the page took an extra 10 seconds to load the page, and as far as I could tell, 5 of those seconds where Chrome stopping and resuming the JS execution 100 times, so there was nothing we could do about that.

The remaining open questions are:

  • Does DOMDebugger.setInstrumentationBreakpoint has the same performance as .*:0:0?
  • How many customers have project with 100s of JavaScript files?

I don't have those answers...

Contributor

digeff commented Sep 28, 2017

@auchenberg We never tested the performance of that flag. We did test the performance of using a breakpoint on .*:0:0 to break on the first line of each and every file, which is similar to that command.

When using .*:0:0 in a project with 100s JavaScript file, the page took an extra 10 seconds to load the page, and as far as I could tell, 5 of those seconds where Chrome stopping and resuming the JS execution 100 times, so there was nothing we could do about that.

The remaining open questions are:

  • Does DOMDebugger.setInstrumentationBreakpoint has the same performance as .*:0:0?
  • How many customers have project with 100s of JavaScript files?

I don't have those answers...

@auchenberg

This comment has been minimized.

Show comment
Hide comment
@auchenberg

auchenberg Sep 28, 2017

Contributor

Given that this core library is shared between Node, Chrome and other runtimes we should be careful about enabling this feature by default, if there's any performance overhead with larger amount of files. Ex: smaller Node projects has typically 500+ files.

@digeff Does the current implementation add a significant overhead?

I'm still a firm believer that we should offload as much as possible to the platform, and if there's a platform issue, like the issue with DOMDebugger.setInstrumentationBreakpoint, it should be fixed by a given platform team, and we shouldn't do workarounds in our debugger.

Contributor

auchenberg commented Sep 28, 2017

Given that this core library is shared between Node, Chrome and other runtimes we should be careful about enabling this feature by default, if there's any performance overhead with larger amount of files. Ex: smaller Node projects has typically 500+ files.

@digeff Does the current implementation add a significant overhead?

I'm still a firm believer that we should offload as much as possible to the platform, and if there's a platform issue, like the issue with DOMDebugger.setInstrumentationBreakpoint, it should be fixed by a given platform team, and we shouldn't do workarounds in our debugger.

Show outdated Hide outdated src/chrome/chromeDebugAdapter.ts
Show outdated Hide outdated src/chrome/chromeDebugAdapter.ts
Show outdated Hide outdated src/chrome/chromeDebugAdapter.ts
Show outdated Hide outdated src/chrome/chromeDebugAdapter.ts
Show outdated Hide outdated src/chrome/chromeDebugAdapter.ts
Show outdated Hide outdated src/chrome/chromeDebugAdapter.ts
Show outdated Hide outdated src/chrome/chromeDebugAdapter.ts
Show outdated Hide outdated src/chrome/chromeDebugAdapter.ts
Show outdated Hide outdated src/chrome/chromeDebugAdapter.ts
Show outdated Hide outdated src/chrome/chromeDebugAdapter.ts
@@ -1004,6 +1094,8 @@ export abstract class ChromeDebugAdapter implements IDebugAdapter {
return this.validateBreakpointsPath(args)
.then(() => {
// Deep copy args to originalArgs
const originalArgs: ISetBreakpointsArgs = JSON.parse(JSON.stringify(args));

This comment has been minimized.

@digeff

digeff Sep 29, 2017

Contributor

Isn't there a better way to do this?

@digeff

digeff Sep 29, 2017

Contributor

Isn't there a better way to do this?

@digeff

This comment has been minimized.

Show comment
Hide comment
@digeff

digeff Oct 2, 2017

Contributor

This feature has a lot of code. What do you guys @roblourens @rakatyal think of putting all the code relevant to this feature in some breakOnLoad.ts file/class and call methods on that file from every other place? That way it'll be easy to find all the relevant code, etc...

Contributor

digeff commented Oct 2, 2017

This feature has a lot of code. What do you guys @roblourens @rakatyal think of putting all the code relevant to this feature in some breakOnLoad.ts file/class and call methods on that file from every other place? That way it'll be easy to find all the relevant code, etc...

Show outdated Hide outdated src/chrome/chromeDebugAdapter.ts
Show outdated Hide outdated src/chrome/chromeDebugAdapter.ts
Show outdated Hide outdated src/chrome/chromeDebugAdapter.ts
Show outdated Hide outdated src/chrome/chromeDebugAdapter.ts
Show outdated Hide outdated src/chrome/chromeDebugAdapter.ts
@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Oct 24, 2017

Member

How can we break up this code a little more? It's hard to break logic out of ChromeDebugAdapter, but there's so much just related to this one feature, that it might make sense to attempt that here. We could have a BreakOnLoadHelper that holds all the state specific to this feature, and gets called into for onPaused, addBreakpoints, etc, as appropriate.

Member

roblourens commented Oct 24, 2017

How can we break up this code a little more? It's hard to break logic out of ChromeDebugAdapter, but there's so much just related to this one feature, that it might make sense to attempt that here. We could have a BreakOnLoadHelper that holds all the state specific to this feature, and gets called into for onPaused, addBreakpoints, etc, as appropriate.

@rakatyal

This comment has been minimized.

Show comment
Hide comment
@rakatyal

rakatyal Oct 25, 2017

Member

@roblourens: It would be tricky since we are deciding whether to pause/continue inside the onPaused and it might lead to some code duplication since we will have to store that state too and add extra code to handle the communication with the helper class we create. But if you feel strongly about this, I can try to do that.

Member

rakatyal commented Oct 25, 2017

@roblourens: It would be tricky since we are deciding whether to pause/continue inside the onPaused and it might lead to some code duplication since we will have to store that state too and add extra code to handle the communication with the helper class we create. But if you feel strongly about this, I can try to do that.

@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Oct 25, 2017

Member

I think we should try. If you want to look over it together and decide what should go where, we can have a call or sit down together. But I see this adding a lot of code to core pathways which just deals with one feature, so I think it will be more readable if we can split it up a little.

Member

roblourens commented Oct 25, 2017

I think we should try. If you want to look over it together and decide what should go where, we can have a call or sit down together. But I see this adding a lot of code to core pathways which just deals with one feature, so I think it will be more readable if we can split it up a little.

@rakatyal

This comment has been minimized.

Show comment
Hide comment
@rakatyal

rakatyal Oct 25, 2017

Member

Okay. I will give it a try and reach out if I need directions.

Member

rakatyal commented Oct 25, 2017

Okay. I will give it a try and reach out if I need directions.

@rakatyal

This comment has been minimized.

Show comment
Hide comment
@rakatyal

rakatyal Oct 30, 2017

Member

@roblourens: Done some refactoring. Please have a look.

Member

rakatyal commented Oct 30, 2017

@roblourens: Done some refactoring. Please have a look.

@roblourens

This is overall really great, and is exactly what I had in mind. The fact that BreakOnLoadHelper is 200 lines long really justifies having a helper class for this feature.

I left a couple comments about moving even more into that class. I think it would be great if we can get rid of all places where chromeDebugAdapter checks the strategy type and does something different. It shouldn't even know that there are two strategies. It's ok to check it for 'none', that will probably simplify a couple things.

Show outdated Hide outdated src/chrome/breakOnLoadHelper.ts
Show outdated Hide outdated src/chrome/chromeDebugAdapter.ts
Show outdated Hide outdated src/chrome/chromeDebugAdapter.ts
Show outdated Hide outdated src/chrome/chromeDebugAdapter.ts
@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Oct 31, 2017

Member

Testing - the easiest way to test this will probably be tests in chrome-debug that launch chrome and use a real page. We could also add unit tests in vscode-chrome-debug-core, and breaking out this helper class could make that easier. Do you think we would benefit from unit tests here?

Member

roblourens commented Oct 31, 2017

Testing - the easiest way to test this will probably be tests in chrome-debug that launch chrome and use a real page. We could also add unit tests in vscode-chrome-debug-core, and breaking out this helper class could make that easier. Do you think we would benefit from unit tests here?

@rakatyal

This comment has been minimized.

Show comment
Hide comment
@rakatyal

rakatyal Nov 1, 2017

Member

Thanks for the great feedback Rob! Addressed most of it. I agree that the best way to test this will be in vscode-chrome-debug and I will start working on it. I am sure we can add some value by adding unit tests but I guess we will be in a better position to assess that once we have the tests written in chrome-debug first. We can then assess the areas that those tests aren't properly covering and then maybe write some unit tests for those. Thoughts?

Member

rakatyal commented Nov 1, 2017

Thanks for the great feedback Rob! Addressed most of it. I agree that the best way to test this will be in vscode-chrome-debug and I will start working on it. I am sure we can add some value by adding unit tests but I guess we will be in a better position to assess that once we have the tests written in chrome-debug first. We can then assess the areas that those tests aren't properly covering and then maybe write some unit tests for those. Thoughts?

@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Nov 1, 2017

Member

That sounds good to me

Member

roblourens commented Nov 1, 2017

That sounds good to me

@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Nov 4, 2017

Member

This looks great, I'll merge this once there are tests in vscode-chrome-debug

Member

roblourens commented Nov 4, 2017

This looks great, I'll merge this once there are tests in vscode-chrome-debug

@roblourens roblourens merged commit 22fbf5f into Microsoft:master Nov 28, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
@auchenberg

This comment has been minimized.

Show comment
Hide comment
@auchenberg

auchenberg Nov 28, 2017

Contributor

Woohu 🎉

Contributor

auchenberg commented Nov 28, 2017

Woohu 🎉

@roblourens roblourens added this to the January 2018 milestone Feb 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment