search

InteractiveAdvertisingBureau / iabtcf-es

Official compliant tool suite for implementing the Transparency and Consent Framework (TCF) v2.0. The essential toolkit for CMPs.
Apache License 2.0
132 stars 96 forks source link

Introduce performance optimizations and new INP metric #436

Open marco-prontera opened 10 months ago

marco-prontera commented 10 months ago

closes #435

The goal of this PR is to introduce a way to allow users of this library to make use of some optimizations that can be made.

This will help us improve INP metrics since the nature of a CMP is very much linked to the new metric, it would also allow us to ensure a better user experience overall.

Here the article where INP is explained https://web.dev/articles/inp

Just let me know if you need some screenshots. @sevriugin Hope in a review soon, thanks in advance

sevriugin commented 10 months ago

We have implemented this maybe we can reuse some ideas here

marco-prontera commented 10 months ago

We have implemented this maybe we can reuse some ideas here

Are you saying to do a similar thing for all the places where I applied memoization?

HeinzBaumann commented 10 months ago

We have implemented this maybe we can reuse some ideas here

I like this implementation a lot better. It does the caching without the need to change any APIs, which is much preferred.

HeinzBaumann commented 10 months ago

There is also this open PR suggesting improvements to the performance to the pub restrictions code: #424 .

marco-prontera commented 10 months ago

We have implemented this maybe we can reuse some ideas here

I like this implementation a lot better. It does the caching without the need to change any APIs, which is much preferred.

So you say to apply the advice suggested by @sevriugin? I added parameters to the APIs to make it possible to decide whether or not to use memoization techniques. Since in the event that a new instance of the TCModel created from the decode of a TCString is necessary, users of the library still have the opportunity to do so. Besides this the addition of these parameters does not change the current behavior so no breaking change is introduced. While if I were to apply the advice suggested, users would be forced to use the clone method to obtain an identical instance of a TCModel. For the use I make of it, honestly I'm fine with having default caching (as suggested), I just want to apply the right choice to avoid possible problems for some users and not introduce breaking changes.

Tell me if you want me to apply the solution suggested by @sevriugin and I will update the PR, otherwise if you have other suggestions we can think about it

marco-prontera commented 6 months ago

Can you review?

sevriugin commented 5 months ago

We have testes this one https://github.com/didomi/iabtcf-es-archived/pull/4 for more then one year so it will be save to apply and no action needed from library users to use this feature, imo, it's better do not change API if we can avoid it, hope it makes sense

marco-prontera commented 5 months ago

We have testes this one didomi#4 for more then one year so it will be save to apply and no action needed from library users to use this feature, imo, it's better do not change API if we can avoid it, hope it makes sense

I will apply the same changes considering that in this PR there are additional improvements in this PR, thanks for the advice 🙏 I will update you asap