DavidFeldhoff / al-codeactions

MIT License
17 stars 8 forks source link

Extension causes high cpu load #141

Closed NKarolak closed 1 year ago

NKarolak commented 2 years ago

Hi David,

upon starting VS Code today morning, I was informed that your extension caused performance issues, and instructed me to share the following with you:

davidfeldhoff.al-codeactions-unresponsive.cpuprofile.txt

Find more details here: https://github.com/microsoft/vscode/wiki/Explain-extension-causes-high-cpu-load

DavidFeldhoff commented 2 years ago

Hi Natalie, thanks for sharing. Let me see what I can do there

NKarolak commented 2 years ago

Maybe useful for you to know: Today, it happened to another extension as well, and there is actually nothing that you can do: https://github.com/jwikman/nab-al-tools/issues/405#issuecomment-1217998318

jwikman commented 2 years ago

Hi @DavidFeldhoff, I just had a quick look at the CPU Profile above, and it seems as the extreme loading time in your case came from loading the applicationInsights module.

image

I'm using the same module, with the same version requirement, and haven't seen that behavior... But we are using webpack to bundle the .js files and produce an as small module as possible. Webpack extracts only the functions that we actually are using in each module - which might explain why we see different behavior.

What you could do, is to look into webpack. That actually improved the performance for us. The downside is that the production code get's minimized and stacktraces etc might be hard to interpret.

DavidFeldhoff commented 2 years ago

Awesome, thanks for the hint @jwikman ! I'll look into it deeper probably next week, then I'll directly try webpack. Thanks!

christianbraeunlich commented 2 years ago

Awesome, thanks for the hint @jwikman ! I'll look into it deeper probably next week, then I'll directly try webpack. Thanks!

If I'm not mistake your VS Code Extension would be the first for AL development that can be used via Browser *<|:-) Am very excited for other extensions to follow this trend. Maybe you could share some thoughts on this one in a blog post / podcast or so, right after? :-)

DavidFeldhoff commented 2 years ago

Do you mean mine or the one of Johannes? At least my extension is dependent on the AZ AL Dev Tools from Andrzej, so he would have to do the first step 😬

jwikman commented 2 years ago

I don't think Andrzej could do that so easy, since he is depending on the AL Language extension (if I'm not mistaken). But maybe if Microsoft decides to make the AL Language extension compatible with Visual Studio Code for the Web, we could see that. But I'm not even sure if it is currently possible...

And I don't believe that I would invest the time needed to get our extension ready for the web (which could be quite a lot) until AL Language extension is, since the AL developers still cannot work with AL on the web.

jwikman commented 2 years ago

@christianbraeunlich webpack does not mean that the extension will be able to be used via Browser, unfortunately. It is only used as a bundler of JavaScript files and modules. Instead of including all dependencies in the vsix file, webpack just picks the functionality that is actually used by the extension from each module and packs all code in a performance wise good way. Since webpack extracts only the code that being used (and minimizes the code), the load time of the extension will improve. Quite a lot if you have a dependency on many modules...

DavidFeldhoff commented 2 years ago

And I don't believe that I would invest the time needed to get our extension ready for the web (which could be quite a lot) until AL Language extension is, since the AL developers still cannot work with AL on the web.

Totally agree with you on this one

DavidFeldhoff commented 1 year ago

Sorry that I haven't implemented something until now. I will make some time next week on thursday or friday

DavidFeldhoff commented 1 year ago

I looked into it yesterday and today, but I wasn't able to get it running with webpack. It always fails because webpack cannot resolve a dependency package of applicationinsights. No clue what's wrong there, so I can't promise to resolve it quickly, sorry. But I have it on my list

jwikman commented 1 year ago

Hi @DavidFeldhoff, have you had a look at my working webpack.config.js to compare differences? (found at https://github.com/jwikman/nab-al-tools/blob/1e12c24045b5ca6a4942e681d3e21eb552cf978e/extension/webpack.config.js#L38)

According https://github.com/microsoft/vscode-extension-telemetry/issues/41#issuecomment-598852991 your issue can be suppressed by adding 'applicationinsights-native-metrics': 'commonjs applicationinsights-native-metrics' to the webpack config, as we have done.

DavidFeldhoff commented 1 year ago

Hi Johannes, thanks again for the fast hint. Yes, I copied it from you without knowing what it was, but I had further issues: image

But after your post here I gave it another try and added these two warnings as well there and now it compiles and can be packed via webpack which is nice! So I created a v1.0.20.vsix for testing purposes and tried to use it, but now my extension can't be started anymore: The error "window is not defined" is coming from one of the dependencies (@opentelemetry/instrumentation, @azure/opentelemetry-instrumentation-azure-sdk) where I now suppress the warning.

image

No clue how I can fix that yet as that is all new stuff for me :/ I'll need more time for that to dig deeper

jwikman commented 1 year ago

There seems to have been some changes in the dependencies for application insights....

I just refreshed all my dependencies with npm update, and now I get the same errors as you do. Module not found: Error: Can't resolve '@opentelemetry/instrumentation' I haven't released a new version for quite some time now, since I've been busy with other things ("life", some calls it).

So I believe that we're in the same boat here. At least we now know that you shouldn't look for inspiration in my extension, and that it is not your fault! 😁

I don't know when I'll get the time to dive too deep into this issue, with TechDays and a new wave coming up (and in the middle of hunting season)...

But I'll let you know as soon as I do any findings!

DavidFeldhoff commented 1 year ago

Oh nice, thanks for the info! I just completed changing my code in that way that it looks now exactly the way you have it (just with the newest packages) and was just getting more desperate as the error remained 😄 I'll have another half an hour a look at it, but then will stop with it for this and the next week and prepare again my session for the TechDays. Thanks for your insights that was already a big help and looking forward to see you in Antwerp!

jwikman commented 1 year ago

Hmmm, adding these two exceptions in the externals section in webpack.config.js seems to have done the trick for me:

    "@opentelemetry/instrumentation": "commonjs @opentelemetry/instrumentation",
    "@azure/opentelemetry-instrumentation-azure-sdk":
      "commonjs @azure/opentelemetry-instrumentation-azure-sdk",

So now it is:

  externals: {
    vscode: "commonjs vscode", // the vscode-module is created on-the-fly and must be excluded. Add other modules that cannot be webpack'ed, 📖 -> https://webpack.js.org/configuration/externals/
    "applicationinsights-native-metrics":
      "commonjs applicationinsights-native-metrics", // ignored because we don't ship native module
    "@opentelemetry/instrumentation": "commonjs @opentelemetry/instrumentation",
    "@azure/opentelemetry-instrumentation-azure-sdk":
      "commonjs @azure/opentelemetry-instrumentation-azure-sdk",
  },

But that was also what you already tried, right?

I can install the vsix, and it seems to load fine, actions available etc. Will test later if I encounter anything else...

Now I really need to shut down my computer... Se you in Antwerp!

DavidFeldhoff commented 1 year ago

Yes, I did exactly the same, but then after I install the vsix my extension crashes while trying to load. I'll see whats the difference then between yours and mine. Thanks for looking into it :)

jwikman commented 1 year ago

Maybe some difference in how we initialize and/or uses application insights?

DavidFeldhoff commented 1 year ago

That's my guess as well. I'll keep you up to date, but it's good news that you got it working!

DavidFeldhoff commented 1 year ago

Nice! I just got it working :) I changed my code exactly like you have it now. Seems that I missed yesterday a bit. And the result upon loading seems to be a good one: Before using webpack: image After using webpack: image

Thanks for your help!

NKarolak commented 1 year ago

Wow, such a difference 😀

jwikman commented 1 year ago

Wow, that improvement was impressive! I'm glad that you sorted this out! 👍

DavidFeldhoff commented 1 year ago

Yes, indeed! But I'll publish it better after the TechDays, just in case something unexpected is happening

jwikman commented 1 year ago

Might be wise... 😁

DavidFeldhoff commented 1 year ago

Version is out right now and it seems it's still working :D :) Just faster. Thanks again for your big help on this one, Johannes :)