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

jsonpatch is really slow and we shouldn't need to merge if there is no default. #4071

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

scr-oath
Copy link
Contributor

No description provided.

@SyntaxNode
Copy link
Contributor

SyntaxNode commented Nov 22, 2024

Yes, it is. We've experimented and had success with jsonutil.Merge as an alternate to jsonpatch for First Party Data. jsonutil.Merge is built on jsoniter taking advantage of that library's callback architecture to customize behavior of ext handling.

It's our intention to replace jsonpatch usage with jsonutil.Merge for a performance increase. Anyone is welcome to put up a PR that beats us to it. :)

No argument against skipping steps which don't apply to everyone. in this case though, how can the default account json ever be nil?

@linux019
Copy link
Contributor

Can we update jsonpatch to https://github.com/evanphx/json-patch/tree/master/v5? V4 uses golang json which is very slow and does a lot of memory allocations

Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 867b85f

relevantdigital

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:46:	Builder				100.0%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:57:	patchBidRequestExt		68.2%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:102:	patchBidImpExt			100.0%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:106:	setTMax				100.0%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:115:	createBidRequest		100.0%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:134:	createJSONRequest		91.7%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:160:	getImpressionExt		100.0%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:176:	buildEndpointURL		100.0%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:185:	buildAdapterRequest		85.7%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:206:	MakeRequests			100.0%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:223:	getImpressionsInfo		100.0%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:235:	getHeaders			100.0%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:255:	getMediaTypeForBidFromExt	100.0%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:266:	getMediaTypeForBid		100.0%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:281:	isSupportedMediaType		83.3%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:295:	MakeBids			81.0%
total:											(statements)			89.9%

Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 54f4257

relevantdigital

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:46:	Builder				100.0%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:57:	patchBidRequestExt		68.2%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:102:	patchBidImpExt			100.0%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:106:	setTMax				100.0%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:115:	createBidRequest		100.0%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:134:	createJSONRequest		91.7%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:160:	getImpressionExt		100.0%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:176:	buildEndpointURL		100.0%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:185:	buildAdapterRequest		85.7%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:206:	MakeRequests			100.0%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:223:	getImpressionsInfo		100.0%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:235:	getHeaders			100.0%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:255:	getMediaTypeForBidFromExt	100.0%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:266:	getMediaTypeForBid		100.0%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:281:	isSupportedMediaType		83.3%
github.com/prebid/prebid-server/v3/adapters/relevantdigital/relevantdigital.go:295:	MakeBids			81.0%
total:											(statements)			89.9%

@scr-oath
Copy link
Contributor Author

scr-oath commented Dec 13, 2024

FWIW, I tried the upgrade to v5 first, and I see that it also uses golang json. I'm curious about the solution in jsonutil - and whether it's been compared/benchmarked against libraries like mergo, gojsonmerge and conjungo if not others…

I also don't see much in the way of speed improvements mentioned in the 4 ⇒ 5 transition (I think going to 5 was just to sort out the fact that the /v5 dir wasn't actually where it needed to be or something.

I will try to do the jsonutil change now… (moving to that will allow picking the best implementation in one place).
@SyntaxNode - I don't see jsonutil.Merge; only jsonutil.MergeClone, which is sufficiently different that I'm not comfortable with a search and replace chore; it seems to take an any object rather than jsonpatch's []byte. Does it work correctly to use jsonutil.MergeClone with a json.RawMessage as the v any destination arg?

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

Successfully merging this pull request may close these issues.

3 participants