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

Remove priority coalescing and PriorityLevel module #11187

Merged
merged 1 commit into from Oct 11, 2017

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Oct 11, 2017

Coalescing is the only feature that depends on PriorityLevel. Since we're not sure if coalescing is even valuable, we'll remove it for now. If it turns out we need it, we can add it back later.

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

Seems legit. Much simpler.

(Check the CI failures.)

@@ -1518,58 +1482,51 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
}
}

function getPriorityContext(
function getExpirationTime(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs a better name. I was really confused what this does.

getExpirationTimeForUpdatesOn(...)?

I don't know...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree it's confusing. Working on a PR that will remove this function entirely, so I'll leave it for now.

Copy link

Choose a reason for hiding this comment

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

Which PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Coalescing is the only feature that depends on PriorityLevel. Since
we're not sure if coalescing is even valuable, we'll remove it for
now. If it turns out we need it, we can add it back later.
@NE-SmallTown
Copy link
Contributor

NE-SmallTown commented Apr 15, 2018

Could I ask that what's the difference between coalescing and batch updates except the interval between those updates? And if "Since we're not sure if coalescing is even valuable", why you write theses codes before, what's your thoughts before?

} else {
// Updates during the render phase should expire at the same time as
// the work that is being rendered.
expirationTime = nextRenderExpirationTime;
Copy link
Contributor

Choose a reason for hiding this comment

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

From the comment 'Updates during the render phase', I'm a little confused why we use else rather than else if (isRendering) here, @acdlite would you mind explain some details? Thanks!

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.

None yet

5 participants