Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fix JsonNode performance regression#104048

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@eiriktsarpalis
Copy link
Member

@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-text-json,@gregsdennis
See info inarea-owners.md if you want to be subscribed.

break;
default:
node=JsonValue.CreateFromElement(refelement,options);
node=newJsonValueOfElement(element,options);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The change looks good, but I'm a little surprised it caused such regressions. It looks like CreateFromElement is also using this constructor but before that is checking ValueKind against the same three cases above. Those branches were enough to cause significant regressions? This must be a super hot path?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It's in the hot path for the lazy conversion ofJsonArray andJsonObject fromJsonElement backed to List and dictionary backed, which is what regressed by this change.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I wonder if theref readonly parameter in the removed method contributed to the regression somehow.

Copy link
Contributor

@andrewjsaidandrewjsaidJun 26, 2024
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

My guess is that it callselement.ValueKind twice, which resolves as follows

publicJsonValueKindValueKind=> TokenType.ToValueKind();`
privateJsonTokenTypeTokenType=> _parent?.GetJsonTokenType(_idx)?? JsonTokenType.None
internalJsonTokenTypeGetJsonTokenType(intindex){CheckNotDisposed();return_parsedData.GetJsonTokenType(index);}
internalJsonTokenTypeGetJsonTokenType(intindex){AssertValidIndex(index);uintunion=MemoryMarshal.Read<uint>(_data.AsSpan(index+NumberOfRowsOffset));return(JsonTokenType)(union>>28);}

plus a small cost to run the switch statement ininternal static JsonValueKind ToValueKind(this JsonTokenType tokenType)

Which is quite a lot of indirection. Possibly changingCreateFromElement to create a localJsonValueKind kind = element.ValueKind; to avoid running the above twice? But still maybe running it once is even a lot here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Created a PR here#104108

eiriktsarpalis reacted with thumbs up emoji
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@stephentoubstephentoubstephentoub approved these changes

@jkurdekjkurdekAwaiting requested review from jkurdek

+1 more reviewer

@andrewjsaidandrewjsaidandrewjsaid left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@eiriktsarpaliseiriktsarpalis

Projects

None yet

Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

[Perf] Linux/x64: 2 Regressions on 6/20/2024 11:40:03 PM

3 participants

@eiriktsarpalis@andrewjsaid@stephentoub

[8]ページ先頭

©2009-2025 Movatter.jp