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

Refactor FPD Merge Functions #3601

Merged
merged 6 commits into from
Apr 26, 2024
Merged

Refactor FPD Merge Functions #3601

merged 6 commits into from
Apr 26, 2024

Conversation

SyntaxNode
Copy link
Contributor

@SyntaxNode SyntaxNode commented Mar 28, 2024

Replaces the FPD merge methods with a smarter implementation.

With the goal of parsing the FPD overlay json only once, the exisitng algorithm cloned the entire app / site / user object before unmarshalling the overlay. A clone is necessary because we share object references between bidder requests to save on allocations, so we need to make a clone to a pointer field before we make a bidder specific change.

Instead of cloning the entire object up front, we can save on allocations and processing time in most cases by tying into the unmarshal process and making a clone as needed. This new approach (now possible with the move to json-iter) also builds ext merging into the unmarshal process and eliminates the need for the extMerger helper.

Benchmarks:

Smallest:
App + ID, ID Overwritten
BenchmarkAppCopy-12    	          976220	        1073 ns/op	     872 B/op	      14 allocs/op
BenchmarkAppMergeClone-12      	 1706986	       700.8 ns/op	     560 B/op	      11 allocs/op

Small:
App - ID + Publisher, ID + Publisher Overwritten
BenchmarkAppCopy-12          	 1883467	       664.5 ns/op	     872 B/op	       8 allocs/op
BenchmarkAppMergeClone-12    	 2491872	       517.9 ns/op	     576 B/op	       6 allocs/op

Medium / Average Case
App - Some Set, ID + Publisher Overwritten
BenchmarkAppCopy-12         	   95799	     12359 ns/op	    8708 B/op	     164 allocs/op
BenchmarkAppMergeClone-12         449222	      2645 ns/op	    1856 B/op	      44 allocs/op

Medium - All Fields Set On Request
App - All Set, ID + Publisher Overwritten
BenchmarkAppCopy-12         	   58051	     21394 ns/op	   14798 B/op	     280 allocs/op
BenchmarkAppMergeClone-12         928129	      1257 ns/op	    1760 B/op	      34 allocs/op

Largest / Worst:
App - All Set, All Overwritten
BenchmarkAppCopy-12         	   34359	     33525 ns/op	   17426 B/op	     405 allocs/op
BenchmarkAppMergeClone-12          33030	     37734 ns/op	   19092 B/op	     435 allocs/op

This new algorithm uses less cpu time and allocates less in almost all cases, except for the worst case. I wouldn't expect the worst case to ever occur, but sharing the benchmark for full transparency. The new algorithm really shines for the medium / average cases.

Copy link
Contributor

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

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

I didn't find the benchmark in the contributed code. Should we include them?

)

err := MergeClone(imp, []byte(`{"banner":nul}`))
require.EqualError(t, err, "cannot unmarshal openrtb2.Imp.Banner: expect ull")
Copy link
Contributor

@guscarreon guscarreon Apr 25, 2024

Choose a reason for hiding this comment

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

Should it read expect null instead of expect ull? Does tryExtractErrorMessage have an off-one bug when building the error message string?

Copy link
Contributor Author

@SyntaxNode SyntaxNode Apr 25, 2024

Choose a reason for hiding this comment

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

This is not a bug in this PR. That is the error message from json-iter. This test is specifically targeting the failure branches of iter.ReadNil. I'll add a comment to clarify.


err := MergeClone(imp, []byte(`{"banner":malformed}`))
require.EqualError(t, err, "cannot unmarshal openrtb2.Imp.Banner: expect { or n, but found m")
})
Copy link
Contributor

Choose a reason for hiding this comment

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

If we add this test case, it'll pass. Do we want MergeClone to have this behavior with empty non null arrays? In order to save memory, should we modify in order to get a nil imp.IframeBuster instead?

	t.Run("nil-existing-empty-incoming", func(t *testing.T) {
		var (
			imp = &openrtb2.Imp{}
		)

		err := MergeClone(imp, []byte(`{"iframeBuster":[]}`))
		require.NoError(t, err)

		assert.Equal(t, []string{}, imp.IframeBuster, "new-val")
	})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. It is proper to parse an empty json array to an empty slice. There is sometimes a difference between a null and empty array.

// token, so must be handled in this decoder.
if iter.ReadNil() {
*(*unsafe.Pointer)(ptr) = nil
d.mapType.UnsafeSet(ptr, d.mapType.UnsafeNew())
Copy link
Contributor

@guscarreon guscarreon Apr 25, 2024

Choose a reason for hiding this comment

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

Is this call initializing ptr to a new empty map? If so, slices and pointers in lines 80 and 104 are simply set to nil. Should slices in line 104 be initialized too?

Copy link
Contributor Author

@SyntaxNode SyntaxNode Apr 25, 2024

Choose a reason for hiding this comment

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

I do not fully understand why setting the pointer to nil isn't sufficient for maps. This is copy I copied/pasted from json-iter, see here. Perhaps it's an incorrect assumption, but I assume this is done for a reason.

Copy link
Contributor

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

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

LGTM

@bsardo bsardo merged commit 36dea2c into prebid:master Apr 26, 2024
3 checks passed
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.

4 participants