Skip to content
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

[preferences][multi root] Potentially exagerated number of calls to getConfigUri() #7631

Closed
marcdumais-work opened this issue Apr 20, 2020 · 9 comments
Assignees
Labels
multi-root issues related to multi-root support performance issues related to performance preferences issues related to preferences

Comments

@marcdumais-work
Copy link
Contributor

marcdumais-work commented Apr 20, 2020

Description

Originally reported in this comment: #7105 (comment)

on current master, just loading the application in a multi-root workspace seems to call the getConfigUri method in the abstract preference provider (packages/preferences/src/browser/abstract-resource-preference-provider.ts) more than 14,000 times. That may be necessary, but it seems like there may be a more efficient way to build the preference object.

14000 times seems excessive.

@akosyakov 's suggestion:

Someone has to measure whether there is really an issue. You could file an issue with a request to investigate it. If it is not hot spot I would prefer don't touch it. Our preference resolution logic is tricky matter.

Reproduction Steps

OS and Theia version:
App: browser example
OS: RHEL 7
Theia version: latest master

Diagnostics:

@marcdumais-work marcdumais-work added preferences issues related to preferences performance issues related to performance multi-root issues related to multi-root support labels Apr 20, 2020
@colin-grant-work
Copy link
Contributor

I can add that OS was RHEL 7 running the browser example app.

@elaihau
Copy link
Contributor

elaihau commented Apr 22, 2020

i found a few things:

  • most getConfigUri() are made from FolderPreferenceProvider, and every root folder in the workspace has 6 FolderPreferenceProviders, i.e., 2 for tasks.json, 2 for launch.json, & 2 for settings.json.

  • caching config uri is not an easy job, as the return value of getConfigUri() is dependent on the status of the file (i.e., tasks.json, launch.json, & settings.json), root folders in the current workspace, and the resourceUri passed into the function. Around 95% of the time, getConfigUri() is called with a resourceUri.

  • surprisingly getConfigUri() does not take much time. I added a timer to track the total time spent on the running the function. none of my 6 providers spent more than 10 ms after I finished the following operations

    • reloading theia by hitting the refresh button of my Chrome browser
    • open the preference editor

@akosyakov
Copy link
Member

We should investigate first whether it causes any bad side effects. If not there is nothing to be done. Particularly in each profiling sessions check whether it is the hottest call.

@colin-grant-work
Copy link
Contributor

Anecdotally, I don't think it really slowed anything down until I tried logging it, and then it was almost certainly the logging that slowed things down, not the logic. But this may also be a chance for me to understand part of this system better. @elaihau , you mention the six providers for each folder; could you help me understand why the launch.json and tasks.json are counted as preference providers along with the settings.json, and why only for the purposes of the folder preference provider?

@elaihau
Copy link
Contributor

elaihau commented Apr 23, 2020

you mention the six providers for each folder; could you help me understand why the launch.json and tasks.json are counted as preference providers along with the settings.json,

I think we access data in launch.json and tasks.json from Preferences mostly due to the goal of achieving vscode compatibility:
Please see the descriptions in #4947, and #6268

The reason that we have 6 providers per root folder is because, not only do we get data from .theia folder but also from .vscode. I believe the purpose behind this, is helping vscode users to more seamlessly migrate from using vscode to theia.

and why only for the purposes of the folder preference provider?

i am not sure if i misunderstand you question.
in a multi root workspace, tasks.json and settings.json are stored under a root folder, and data in those files doesn't have any impact on other root folders in the same workspace.
sorry I can't say anything about launch.json as i have never touched it :)

it was almost certainly the logging that slowed things down, not the logic

Yep i think so too. so in my experiment i don't log anything until the accumulated run time of that function exceeds a threshold, say, 5 ms.

@colin-grant-work
Copy link
Contributor

@elaihau, thanks that's very helpful!

@elaihau
Copy link
Contributor

elaihau commented Apr 24, 2020

We should investigate first whether it causes any bad side effects. If not there is nothing to be done. Particularly in each profiling sessions check whether it is the hottest call.

@akosyakov
As per the profiling tool in firefox, this function takes very little time.

Peek 2020-04-23 22-35

From static analysis, I can see that this function is called every time a preference item is retrieved, updated, or initilized. It might have slowed down the startup a little, but I don't think it has major impacts as it doesn't block anything.

@akosyakov
Copy link
Member

ok, let's move on then

Just think about React virtual DOM rendering i'm pretty sure that there are much many calls to our render method, we are not going to rewrite react because of it.

@elaihau
Copy link
Contributor

elaihau commented May 7, 2020

If there are no objections I am going to close this one in a day.

@elaihau elaihau closed this as completed May 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multi-root issues related to multi-root support performance issues related to performance preferences issues related to preferences
Projects
None yet
Development

No branches or pull requests

4 participants