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(tracehouse): allow child to start <1ms before parent #9786

Merged
merged 7 commits into from
Oct 15, 2019

Conversation

patrickhulce
Copy link
Collaborator

Summary
Addresses one of the 3 remaining cases for Fatal trace logic error - child cannot end after parent error.

Related Issues/PRs
#7764, depends on #9785

@patrickhulce patrickhulce force-pushed the forgive_incorrect_nesting_order branch from b6ab7fe to 4cf372a Compare October 7, 2019 20:38
@vercel vercel bot temporarily deployed to staging October 7, 2019 20:38 Inactive
it('should handle nested tasks of the same name', () => {
/*
An artistic rendering of the below trace:
████████████████TaskA██████████████████
Copy link
Member

Choose a reason for hiding this comment

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

s/TaskA/SameName/g

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

a few nits but yah lgtm

lighthouse-core/lib/tracehouse/main-thread-tasks.js Outdated Show resolved Hide resolved
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM2

nextTask.startTime - currentTask.startTime < 1000 &&
!currentTask.children.length
) {
// The parent started less than 1ms before the child, we'll let it slide by swapping the two,
Copy link
Member

@brendankenny brendankenny Oct 15, 2019

Choose a reason for hiding this comment

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

should this be the opposite (parent<->child)? (at least in terms of what appears to be the case before the swap)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe we should give up using 'parent' and 'child' in these swapping contexts 😆 this is written in terms of the actual structure of the trace events, not what we the current object structure might reflect.

maybe we take the same variable name approach from before and just slap actual before these two?

!currentTask.children.length
) {
// The parent started less than 1ms before the child, we'll let it slide by swapping the two,
// and increasing the duration of the parent. Below is an artistic rendition of this situation.
Copy link
Member

Choose a reason for hiding this comment

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

this is the rendition after the swap, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

errr, kind of. once we fix everything nextTask and currentTask labels will be switched so the diagram looks correct. at this exact moment in the code there is no heirarchy relationship whatsoever between nextTask and currentTask this diagram is attempting to take the true structure of things and apply the variable labels we currently have for them.

open to whatever explanation you think fits best and doesn't confuse me too :)

@patrickhulce
Copy link
Collaborator Author

@brendankenny made a few comment tweaks I hope it makes sense!

@brendankenny
Copy link
Member

@brendankenny made a few comment tweaks I hope it makes sense!

looks great!

@patrickhulce patrickhulce merged commit 85b8a92 into master Oct 15, 2019
@patrickhulce patrickhulce deleted the forgive_incorrect_nesting_order branch October 15, 2019 20:37
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