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

Core: start yielding control of the main thread #12025

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

Conversation

dgirardi
Copy link
Collaborator

Type of change

  • Feature

Description of change

  • Make GreedyPromise optional: it can be excluded with gulp build --disable GREEDY and/or overridden with pbjs.Promise = window.Promise (before Prebid loads)
  • Refactor tests so that they can pass on both vanilla and greedy promises

Since promises are already used in many places, "vanilla" promises should have the effect of breaking up long running tasks (#10062), but they are also potentially breaking - the timing of configuration taking effect, event handlers running, etc all change (relative to when their setup code runs).

The plan is to do some testing and, if necessary, find more tasks to split using promises - so that any such change only affects those that opt out of GreedyPromise.

Copy link

Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:

Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀

Copy link

Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:

Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀

Copy link

Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:

Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀

Copy link

Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:

Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀

Copy link

Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:

Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀

Copy link

Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:

Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀

Copy link

Tread carefully! This PR adds 1 linter warning (possibly disabled through directives):

  • libraries/consentManagement/cmUtils.js (+1 warning)

Copy link

Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:

Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀

@gilbertococchi
Copy link

Hi @dgirardi, are you still committed to move this branch forward?

I just wanted to share that the scheduler.yield Yield method to allow 3Ps or 1P Script led Tasks to Yield while ensuring continuation (so without causing delay into Ad bids auction) is now widely available in Chrome.

I think that may be a good fit for this branch proposal for interested publishers to test it out if applicable.

@patmmccann
Copy link
Collaborator

Raptive is planning some quantitative tests on this branch in November

@mmoschovas mmoschovas removed their assignment Nov 6, 2024
@mmoschovas mmoschovas removed their request for review November 6, 2024 15:46
@gilbertococchi
Copy link

Thanks for your answer @patmmccann!

If you have feedback or questions about yield best practices on the Yield I am available to answer those questions.

@marco-prontera
Copy link

It would be interesting to test this improvement. Any news on the progress?

@patmmccann
Copy link
Collaborator

It would be interesting to test this improvement. Any news on the progress?

Ironically, we're awaiting people like you to test the branch on live traffic before merging it

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

Successfully merging this pull request may close these issues.

8 participants