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

Log when debugger drops large variables. #74148

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

baptr
Copy link
Contributor

@baptr baptr commented Mar 1, 2023

It's confusing to see <null> objects in the debugger stack inspector when they should be assigned, and hard to track down why it might be.

It turns out the remote debugger's DebuggerMarshalls serialize functions default to transferring at most 1MiB of data. Before this, it would silently drop anything that would be too large, and display them as <null>

Before:
image
(no logs of any sort)

After:
(same stack view), but:

W 0:00:00:0953   TestScene.gd:12 @ _ready(): Variable w too large to serialize for debugger: 1059020 > 1048576
  <C++ Source>   core/debugger/debugger_marshalls.cpp:83 @ serialize()
  <Stack Trace>  TestScene.gd:12 @ _ready()
W 0:00:00:0999   TestScene.gd:12 @ _ready(): Variable warrior_scene too large to serialize for debugger: 1503520 > 1048576
  <C++ Source>   core/debugger/debugger_marshalls.cpp:83 @ serialize()
  <Stack Trace>  TestScene.gd:12 @ _ready()
W 0:00:01:0003   TestScene.gd:12 @ _ready(): Variable warrior_node too large to serialize for debugger: 1059020 > 1048576
  <C++ Source>   core/debugger/debugger_marshalls.cpp:83 @ serialize()
  <Stack Trace>  TestScene.gd:12 @ _ready()

Related: #39465
Possibly related: #38965, #36801

@Faless
Copy link
Collaborator

Faless commented Mar 1, 2023

I don't think we should make this spammy, the inspector should instead print something like too big in place of null.
Which means we should probably add property hint and hint_string to the ScriptStackVariable message (like done in scene/debugger/scene_debugger.cpp for SceneDebuggerObject).

@baptr
Copy link
Contributor Author

baptr commented Mar 1, 2023

I don't think we should make this spammy, the inspector should instead print something like too big in place of null. Which means we should probably add property hint and hint_string to the ScriptStackVariable message (like done in scene/debugger/scene_debugger.cpp for SceneDebuggerObject).

Perfect, thanks for the reference. I was trying to find an existing type to reflect an error like that. I'll take a look at setting that up tomorrow.

@Chaosus Chaosus added this to the 4.x milestone Mar 1, 2023
@isaaccp
Copy link
Contributor

isaaccp commented Mar 1, 2023

Thanks for looking into this @baptr ! I was the one seeing this behavior yesterday and agree that showing something directly in the debugger would be the best option.

@Faless do you think it's reasonable to add an editor/project setting to be able to change this 1MB limit? It seems very easy to reach :) i am happy to look into it if so.

@Calinou
Copy link
Member

Calinou commented Mar 1, 2023

With the new debugger code in 4.0, could larger variables be handled out of the box? It may be a good opportunity to raise the limit to 4 MB or even 8 MB.

It's confusing to see `<null>` objects in the debugger inspector when
they should be assigned, and hard to track down why it might be.

It turns out the remote debugger's DebuggerMarshalls serialize functions
default to transferring at most 1MiB of data. Before this, it would
silently drop anything that would be too large.

- Stop recursively encoding objects when checking their size, since
  they're not actually sent that way.
- Change PROPERTY_HINT_OBJECT_TOO_BIG to a more generic
  PROPERTY_HINT_ENCODING_ERROR and use hint_string to specify the issue.
- Plumb ENCODING_ERROR hints (but no others) through the remote
  debugger.
- Surface the new ENCODING_ERROR hint_string in the debugger.
@baptr baptr requested review from a team as code owners March 2, 2023 10:53
@baptr
Copy link
Contributor Author

baptr commented Mar 2, 2023

I hacked around with it a bit...

It turns out the 1MiB check is a bit bogus anyway, since the true in the length check does a full recursive encoding, unlike what's actually used when the variant is transmitted:

Error err = encode_variant(var, nullptr, len, true);

vs
encode_variant(var, buf + 4, size);

Changing that gives a lot more headroom for the more common cases, but you can still trip the limit with large flat variants like a huge array. So I pushed a bit more on giving some feedback when there is an encoding problem...

Unfortunately I didn't find a great way to surface property hints for conveying them. The existing PROPERTY_HINT_OBJECT_TOO_BIG wasn't actually honored by anything. It was a small enough change, but feels a bit unclean, to add error hint support to EditorPropertyNil which is used for the fallback Variant() we end up marshalling when there's an issue...

But that only helps for the debugger's stack view, not the remote scene tree. We'd need a different solution for Arrays (and probably other custom rendered types) that keep their type information in the remote scene tree, which render as (Nil) Array currently.

Here's what my current sketch in 5f19775 produces if I omit the first encode_variant tweak to trigger more <too big>s:

image

@baptr
Copy link
Contributor Author

baptr commented Mar 2, 2023

Here's the same as a draft PR with split commits, if that's useful: #74230

@DinoMC
Copy link

DinoMC commented Dec 25, 2024

I'm confused, is this still waiting for reviewers or is there another issue?

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

Successfully merging this pull request may close these issues.

6 participants