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

[V3] Fix build indeterminism by sorting entries #319

Merged
merged 11 commits into from
Feb 7, 2025
38 changes: 31 additions & 7 deletions crates/atlaspack/src/requests/asset_graph_request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ struct AssetGraphBuilder {
receiver: ResultReceiver,
asset_request_to_asset_idx: HashMap<u64, NodeIndex>,
waiting_asset_requests: HashMap<u64, HashSet<NodeIndex>>,
entry_dependencies: Vec<(String, NodeIndex)>,
}

impl AssetGraphBuilder {
Expand All @@ -71,6 +72,7 @@ impl AssetGraphBuilder {
receiver,
asset_request_to_asset_idx: HashMap::new(),
waiting_asset_requests: HashMap::new(),
entry_dependencies: Vec::new(),
}
}

Expand Down Expand Up @@ -131,6 +133,22 @@ impl AssetGraphBuilder {
}
}

// Connect the entries to the root node in the graph. We do this in
// alphabetical order so it's consistent between builds.
//
// Ideally, we wouldn't depend on edge order being consistent between builds
// and instead rely on in-place sorting or similar to ensure deterministic
// builds. However, as the rest of the code base (bundling, runtimes,
// packaging, etc) relies on the deterministic edge order and it's very
// complicated/risky to fix all the places that would be affected we'll keep it that
// way for now.
self
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem isn't on this graph, is on the other parts of the codebase which are depending on the order these edges and nodes exist in.

There is one known place which does this added in https://github.com/parcel-bundler/parcel/pull/9706/files

Here, instead of assetGraph.dfsFast which will depend on the order of nodes on the graph, we should be sorting them by some preset value (like ID or path). This is tracked by AFB-406 since that change was made.

I'd rather we try to fix that first. You would need to sort in that part instead of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, I attempted to fix the individual places we rely on edge order but it became a risky and complicated fix that will take time to get right.

I've added a comment to explain the situation and we can look to remove the ordering code once the bundler has been replaced.

.entry_dependencies
.sort_by_key(|(entry, _)| entry.clone());
for (_, node_index) in self.entry_dependencies.iter() {
self.graph.add_edge(&self.graph.root_node(), node_index);
}

Ok(ResultAndInvalidations {
result: RequestResult::AssetGraph(AssetGraphRequestOutput { graph: self.graph }),
invalidations: vec![],
Expand Down Expand Up @@ -192,10 +210,12 @@ impl AssetGraphBuilder {
.request_context
.queue_request(asset_request, self.sender.clone());
} else if let Some(asset_node_index) = self.asset_request_to_asset_idx.get(&id) {
// We have already completed this AssetRequest so we can connect the
// Dependency to the Asset immediately
self.graph.add_edge(&dependency_idx, asset_node_index);
self.propagate_requested_symbols(*asset_node_index, dependency_idx);
if !self.graph.has_edge(&dependency_idx, asset_node_index) {
// We have already completed this AssetRequest so we can connect the
// Dependency to the Asset immediately
self.graph.add_edge(&dependency_idx, asset_node_index);
self.propagate_requested_symbols(*asset_node_index, dependency_idx);
}
} else {
// The AssetRequest has already been kicked off but is yet to
// complete. Register this Dependency to be connected once it
Expand Down Expand Up @@ -282,8 +302,10 @@ impl AssetGraphBuilder {
// for this AssetNode to be created
if let Some(waiting) = self.waiting_asset_requests.remove(&request_id) {
for dep in waiting {
self.graph.add_edge(&dep, &asset_idx);
self.propagate_requested_symbols(asset_idx, dep);
if !self.graph.has_edge(&dep, &asset_idx) {
self.graph.add_edge(&dep, &asset_idx);
self.propagate_requested_symbols(asset_idx, dep);
}
}
}
}
Expand Down Expand Up @@ -405,10 +427,12 @@ impl AssetGraphBuilder {
for target in targets {
let entry =
diff_paths(&entry, &self.request_context.project_root).unwrap_or_else(|| entry.clone());
let entry = entry.to_str().unwrap().to_string();

let dependency = Dependency::entry(entry.to_str().unwrap().to_string(), target);
let dependency = Dependency::entry(entry.clone(), target);

let dep_node = self.graph.add_entry_dependency(dependency.clone());
self.entry_dependencies.push((entry, dep_node));

let request = PathRequest {
dependency: Arc::new(dependency),
Expand Down
5 changes: 4 additions & 1 deletion crates/atlaspack_core/src/asset_graph/asset_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,6 @@ impl AssetGraph {
pub fn add_entry_dependency(&mut self, dependency: Dependency) -> NodeIndex {
let is_library = dependency.env.is_library;
let dependency_idx = self.add_dependency(dependency);
self.add_edge(&self.root_node_index.clone(), &dependency_idx);

if is_library {
if let Some(dependency_node) = self.get_dependency_node_mut(&dependency_idx) {
Expand All @@ -186,6 +185,10 @@ impl AssetGraph {
dependency_idx
}

pub fn has_edge(&mut self, from_idx: &NodeIndex, to_idx: &NodeIndex) -> bool {
self.graph.contains_edge(*from_idx, *to_idx)
}

pub fn add_edge(&mut self, from_idx: &NodeIndex, to_idx: &NodeIndex) {
self.graph.add_edge(*from_idx, *to_idx, ());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use atlaspack_core::types::{Asset, Dependency};
/// nodes: Array<JsBuffer>,
/// }
/// ```
#[tracing::instrument(level = "info", skip_all)]
pub fn serialize_asset_graph(env: &Env, asset_graph: &AssetGraph) -> anyhow::Result<JsObject> {
// Serialize graph nodes in parallel
let nodes = asset_graph
Expand Down
56 changes: 56 additions & 0 deletions packages/core/integration-tests/test/bundler.js
Original file line number Diff line number Diff line change
Expand Up @@ -2341,4 +2341,60 @@ describe('bundler', function () {
await run(splitBundle);
},
);

it('should produce deterministic builds with multiple entries', async () => {
await fsFixture(overlayFS, __dirname)`
deterministic-builds
shared-one.js:
export const one = 'one';
shared-two.js:
export const two = 'two';
one.js:
import {one} from './shared-one';
import {two} from './shared-two';
sideEffectNoop(one + two);
entry-one.html:
<script type="module" src="./one.js" />
two.js:
import {two} from './shared-two';
import {one} from './shared-one';
sideEffectNoop(two + one);
entry-two.html:
<script type="module" src="./two.js" />
package.json:
{
"@atlaspack/bundler-default": {
"minBundleSize": 0,
"manualSharedBundles": [{
"name": "shared",
"assets": ["shared*.js"]
}]
}
}
yarn.lock:
`;

let build = () =>
bundle(
[
path.join(__dirname, 'deterministic-builds/entry-one.html'),
path.join(__dirname, 'deterministic-builds/entry-two.html'),
],
{
inputFS: overlayFS,
},
).then((b) =>
b
.getBundles()
.map((b) => b.filePath)
.sort(),
);

let bundles = await build();

for (let i = 0; i < 10; i++) {
let compareBundles = await build();
assert.deepEqual(bundles, compareBundles);
}
});
});
5 changes: 4 additions & 1 deletion packages/core/integration-tests/test/scope-hoisting.js
Original file line number Diff line number Diff line change
Expand Up @@ -6177,8 +6177,11 @@ describe('scope hoisting', function () {
let contents = await outputFS.readFile(indexBundle.filePath, 'utf8');
assert(contents.includes('$parcel$global.rwr('));

let indexHtmlBundle = nullthrows(
b.getBundles().find((b) => /index.*\.html/.test(b.filePath)),
);
let result;
await run(b, {
await runBundle(b, indexHtmlBundle, {
result: (r) => {
result = r;
},
Expand Down