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

Large dictionaries cause the UI to become unresponsive #80959

Open
njones0093 opened this issue Aug 24, 2023 · 14 comments
Open

Large dictionaries cause the UI to become unresponsive #80959

njones0093 opened this issue Aug 24, 2023 · 14 comments

Comments

@njones0093
Copy link

njones0093 commented Aug 24, 2023


Bugsquad note: This issue has been confirmed several times already. No need to confirm it further.


Godot version

4.0.1 Stable

System information

Windows 11 Vulkan API 1.3.224 - Forward+ - Using Vulkan Device #0: NVIDIA - NVIDIA GeForce RTX 3050 Laptop GPU

Issue description

The company I work for are using Godot for a commercial software project rather than a game and as such we deal with large dictionaries (sometimes have a size in the 1000s). When hitting a breakpoint, if we hover over the variable which is storing the dictionary, it makes the Godot UI to become unresponsive. The only way to get out of it is to hard-crash the editor (using Task Manager or closing the console window, if open).

Using the Stack Variable inspector does not cause this same issue.

Steps to reproduce

  • Create a very large dictionary with lots of data, either creating it in code or loading it from a JSON file (the dictionary does not have to be large in file size, just have a lot of elements)
  • Place a breakpoint at a point in the code where the value of the variable which stores the dictionary is visible by hovering over it

Minimal reproduction project

This is the project I made to reproduce this, putting a breakpoint on the print(1). Unfortunately, this also showed another bug. Even variables which are not null show as null when hovering over them and when inspecting them in the stack. Printing them does show their value.
dictionary crash.zip

@AThousandShips
Copy link
Member

Can you confirm this on 4.0.4 or another more recent version?

@njones0093
Copy link
Author

Can you confirm this on 4.0.4 or another more recent version?

Hi, I have just tried on Godot 4.1.1 and it still crashes my interface. The difference between versions is that in 4.1.1 the interface does eventually close while leaving my runtime window open. On 4.0.1 it crashes the interface and nothing else happens.

@AThousandShips
Copy link
Member

And on 4.0.4?

@njones0093
Copy link
Author

njones0093 commented Aug 24, 2023

And on 4.0.4?

Tested on 4.0.4 mono and on standard 4.0.4 and it doesn't crash the editor with the same dictionary that it crashes the others with

Edit: Scrap that, the editor stopped working when I tried to remove the breakpoint and continue the program. The UI still crashed, leaving the runtime running

@jsjtxietian
Copy link
Contributor

jsjtxietian commented Aug 25, 2023

Even variables which are not null show as null when hovering over them and when inspecting them in the stack. Printing them does show their value.

I can reproduce this bug. Also when I double click the large-file.json, the editor freezed. I will take a look about what happened. I like dealing with performance issues.

And I think there are three problems here need testing and solving:

  1. hovering a variable will show null in certain cases (small file works fine, only the large one caused lsp to return null)
  2. After fixing 1, check whether hover a large dictionary will cause crash
  3. double click the large-file.json will freeze the editor

Editor version: Godot v4.2.dev (6f90b23) - Windows 10.0.22621 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3060 Ti (NVIDIA; 31.0.15.3699) - 12th Gen Intel(R) Core(TM) i7-12700F (20 Threads)

@tom-jk
Copy link

tom-jk commented Aug 25, 2023

Looks similar to #64569.

@jsjtxietian
Copy link
Contributor

jsjtxietian commented Aug 26, 2023

  1. hovering a variable will show null in certain cases (small file works fine, only the large one caused lsp to return null)

About this problem :

The serialize function for remote debug has a max size limit:Array serialize(int max_size = 1 << 20); // 1 MiB default.

But the length of the json dictionary is 3353848, bigger than 1 << 20, so it just using a new Variant as value instead.

image

I think there might be two ways of fixing this:

  1. Increase the limit a little bit
  2. Instead of returning a nil value, return a more friendly info such as 'Value (along with type) too big for showing'

Update: Solved by #74148

@njones0093
Copy link
Author

  1. hovering a variable will show null in certain cases (small file works fine, only the large one caused lsp to return null)

About this problem :

The serialize function for remote debug has a max size limit:Array serialize(int max_size = 1 << 20); // 1 MiB default.

But the length of the json dictionary is 3353848, bigger than 1 << 20, so it just using a new Variant as value instead.

image

I think there might be two ways of fixing this:

  1. Increase the limit a little bit
  2. Instead of returning a nil value, return a more friendly info such as 'Value (along with type) too big for showing'

Any ideas?

That makes a lot of sense. I think it would be more user friendly if the tool tip showed that the value is too large, rather than null. It has been the source of some confusion in our projects, especially for our non-programmers. Its unfortunate that the actual value can't be displayed

@jsjtxietian
Copy link
Contributor

jsjtxietian commented Aug 26, 2023

  1. After fixing 1, check whether hover a large dictionary will cause crash

About this, when the test file' szie is close to 1<<20 , the crash will happen. It's a vulkan related crash (crashed when create buffer), and I strongly suspect it's because the vertex for the flat style box is too large as the image shown.

image

I think I can make a seperat PR for this and truncate the flat style box in advance?

@jsjtxietian
Copy link
Contributor

jsjtxietian commented Sep 2, 2023

  1. double click the large-file.json will freeze the editor

And the last piece of this problem, I find the hotspot like this:

image

This is the related code:

// Update height.
const int old_height = text.write[p_line].height;
const int wrap_amount = get_line_wrap_amount(p_line);
int height = font_height;
for (int i = 0; i <= wrap_amount; i++) {
    height = MAX(height, text[p_line].data_buf->get_line_size(i).y);
}
text.write[p_line].height = height;

Inside get_line_wrap_amount, it calles _shape_lines which is very time consuming just to get the line wrap amount (usually zero), I believe it can be optimized. Open a 470 kilobytes json file takes 2-3 seconds which is probably fine, but open a 20MB json file will completely freeze the editor for a long time and comsume tons of memory, this can be optimized.

@Paulb23 Any thoughts? I guess maybe at least for json file we can just ignore the get_line_wrap_amount call ? Or we check if autowrap is false then we skip this function ?

@Calinou
Copy link
Member

Calinou commented Sep 2, 2023

Many editors disable softwrapping for a given file when opening a file that is too large or has a longest line that is over 16,384 characters long or so. This is probably what I'd do.

@jsjtxietian
Copy link
Contributor

jsjtxietian commented Sep 3, 2023

I dig into the code and find it's non trivial to simply add suppot for this, the problem lies in _shape_lines, this function is too slow and many function calls depend on it. If I get rid of get_line_wrap_amount, get_size will call _shape_lines too.

Have to pick up the hard way, find out why _shape_lines is too slow.

@bruvzg
Copy link
Member

bruvzg commented Oct 6, 2023

Any thoughts? I guess maybe at least for json file we can just ignore the get_line_wrap_amount call ? Or we check if autowrap is false then we skip this function ?

It probably won't make any difference, _shape will be called anyway as soon data_buf is accessed (line drawn or any measurement taken). The only reasonable thing is likely adding support for text processing in the background thread.

@FoxyFox909
Copy link

Still happening as of Godot 4.2 Stable Release.

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

No branches or pull requests

8 participants