perf: improve performance of purpose restriction encoder [encode] #424
Conversation
|
@marco-prontera quick remark - did not follow up on this given you patched it. Your original new implementation only broke I think because during encoding resets the GVL filter during encoding (the check of the ranges against the GVL failed).I assume this should not happen with clone but had not time to think about it, not do it get why the clone is done anyhow |
@asr-enid Can you prove what you say through PR? |
marco-prontera
changed the title
|
Hi @sevriugin, I have updated the PR adding a further improvement in encoding performance with the use of pub restrictions. Essentially it involves skipping useless loops with vendorIds that aren't there, this ensures we avoid causing long JS tasks that could negatively impact the new Google INP metric. |
|
@marco-prontera Thanks Marco, I will take a look |
marco-prontera
force-pushed
the
perf-purpose-restriction
branch
from
c21157b
to
4be6f99
Compare
|
Hi @sevriugin could you review this, please? |
|
Can you review? |
|
|
||
| return nextId; | ||
| if (lastIndex - firstIndex > 0) { |
There was a problem hiding this comment.
Could this one be true if first index is equal -1 -> not found?
There was a problem hiding this comment.
Yes that condition is required only to check if we are at the "end" of the used GVL vendor list in fact in line 26 you see we are taking the nextIndex the one that is specified in the GVL vendor list.
This makes possible to skip every extra loop in the purpose restriction encode process.
|
|
||
| } | ||
| const nextIndex = gvlVendorIds.indexOf(vendorId + 1); |
There was a problem hiding this comment.
vendorId + 1 could be out of range if vendorId is the last vendor in the GVL, right?
There was a problem hiding this comment.
If the vendorId is the last vendor in the GVL this block won't be executed, in fact the vendorId is the latest this function will return itself as "special case". See return statement at line 35
|
|
||
| const {nextVendorId, index} = nextGvlVendor(vendorId, vendors[len - 1]); | ||
|
|
||
| if (vendors[i + 1] > nextVendorId) { |
There was a problem hiding this comment.
vendors[i+1] is always defined ?
There was a problem hiding this comment.
yes, in fact if it is not defined the relevant block of code will not be executed. See the condition at line 74 and the condition at line 61
|
@marco-prontera added some question? hope it makes sense, cheers |
There was a problem hiding this comment.
@sevriugin Hope everything makes sense to you. I've been using this optimisation for about 7 months without any problem. Previously if you remember I introduce more tests for this part that enables us to do this types of changes.
Let me know, I'd love to help even more
|
|
||
| const {nextVendorId, index} = nextGvlVendor(vendorId, vendors[len - 1]); | ||
|
|
||
| if (vendors[i + 1] > nextVendorId) { |
There was a problem hiding this comment.
yes, in fact if it is not defined the relevant block of code will not be executed. See the condition at line 74 and the condition at line 61
|
|
||
| } | ||
| const nextIndex = gvlVendorIds.indexOf(vendorId + 1); |
There was a problem hiding this comment.
If the vendorId is the last vendor in the GVL this block won't be executed, in fact the vendorId is the latest this function will return itself as "special case". See return statement at line 35
|
|
||
| return nextId; | ||
| if (lastIndex - firstIndex > 0) { |
There was a problem hiding this comment.
Yes that condition is required only to check if we are at the "end" of the used GVL vendor list in fact in line 26 you see we are taking the nextIndex the one that is specified in the GVL vendor list.
This makes possible to skip every extra loop in the purpose restriction encode process.
|
Ok, thanks for explanation, LGTM |
The goal of this PR is to improve performance of purpose restriction encoder.
Here a link to check https://codesandbox.io/s/tender-colden-nzmdc6
ps: @sevriugin This is an addition to the last PR that was merged but I didn't have time to update it before it was merged into master. Since the new version hasn't been released yet I think we're in time to merge it