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

feat(gatsby): PQR: merge data dependencies from workers to the main process #32305

Merged
merged 7 commits into from
Jul 19, 2021

Conversation

vladar
Copy link
Contributor

@vladar vladar commented Jul 9, 2021

Description

We need to sync parts of the build state from the query worker to the main process. This PR "replays" selected actions from workers in the main process when each query chunk is finished.

The alternative approach would be to merge state at the end of query running but this may be error-prone and harder to maintain because requires that each reducer interested in query events additionally "knows" about this potential merge step.

The upside of state merging is that it can be more performant and has less IPC communication overhead. So we may get back to it eventually if action replaying proves to be too expensive.

[ch33833]

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jul 9, 2021
@vladar vladar added topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine) and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jul 9, 2021
@pieh
Copy link
Contributor

pieh commented Jul 9, 2021

Just so there is papertrail on PR:

keep in mind PAGE_QUERY_RUN action now is dispatched in workers and we would need it in main process to keep track of static query results chaging for incremental html builds in

case `PAGE_QUERY_RUN`: {
// Despite action name, this action is actually emitted for both page and static queries.
// In here we actually only care about static query result (particularly its hash).
// We don't care about page query result because we don't actually use page query result
// directly when generating html. We care about page-data (which contains page query result).
// Handling of page-data that transitively handles page query result is done in handler for
// `ADD_PAGE_DATA_STATS` action.
if (!action.payload.isPage) {
// static query case
let staticQueryResult = state.trackedStaticQueryResults.get(
action.payload.queryHash
)
if (!staticQueryResult) {
staticQueryResult = {
dirty: FLAG_DIRTY_STATIC_QUERY_FIRST_RUN,
staticQueryResultHash: action.payload.resultHash,
}
state.trackedStaticQueryResults.set(
action.payload.queryHash,
staticQueryResult
)
} else if (
staticQueryResult.staticQueryResultHash !== action.payload.resultHash
) {
staticQueryResult.dirty |= FLAG_DIRTY_STATIC_QUERY_RESULT_CHANGED
}
}
return state
}

@vladar vladar force-pushed the vladar/worker-data-deps branch from d1af064 to cb80dab Compare July 15, 2021 13:21
@vladar
Copy link
Contributor Author

vladar commented Jul 15, 2021

Yeah, after some thinking I've changed the implementation to just replaying actions after each chunk is done. If data dependencies will be too expensive to sync this way - we can do a hybrid approach - forward/replay some actions and merge expensive parts all at once in the end.

But first, let's try the simple approach with just replaying actions.

@vladar vladar marked this pull request as ready for review July 15, 2021 14:20
Copy link
Contributor

@LekoArts LekoArts left a comment

Choose a reason for hiding this comment

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

Two smaller nits

@vladar vladar merged commit bdb9352 into master Jul 19, 2021
@vladar vladar deleted the vladar/worker-data-deps branch July 19, 2021 20:27
LekoArts pushed a commit that referenced this pull request Jul 20, 2021
…rocess (#32305)

* feat(gatsby): PQR: merge data dependencies from workers to the main process

* Move worker state merging to a standalone step

* revert acciental change

* different approach: replay actions vs merging state

* revert unneded changes

* do not use inline snapshot

* Update packages/gatsby/src/utils/worker/__tests__/queries.ts

Co-authored-by: Lennart <[email protected]>

Co-authored-by: Lennart <[email protected]>
(cherry picked from commit bdb9352)
LekoArts pushed a commit that referenced this pull request Jul 20, 2021
…rocess (#32305) (#32438)

* feat(gatsby): PQR: merge data dependencies from workers to the main process

* Move worker state merging to a standalone step

* revert acciental change

* different approach: replay actions vs merging state

* revert unneded changes

* do not use inline snapshot

* Update packages/gatsby/src/utils/worker/__tests__/queries.ts

Co-authored-by: Lennart <[email protected]>

Co-authored-by: Lennart <[email protected]>
(cherry picked from commit bdb9352)

Co-authored-by: Vladimir Razuvaev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants