Skip to content

Commit 4b46467

Browse files
committed
refactor(project): Rename matchResourceMetadataStrict to isResourceUnchanged
Drop the unused matchResourceMetadata variant (no longer used) and rename matchResourceMetadataStrict to isResourceUnchanged.
1 parent 7130ad5 commit 4b46467

8 files changed

Lines changed: 65 additions & 179 deletions

File tree

.claude/skills/incremental-build/architecture.md

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -218,14 +218,15 @@ Key operations:
218218
- `removeResources(paths)`: Remove resources, recompute affected hashes
219219
- `_computeHash(node)`: Recursive hash computation
220220

221-
The `matchResourceMetadataStrict` utility (`utils.js`) determines if a resource is "unchanged" using a tiered comparison (cheapest first):
221+
The `isResourceUnchanged` utility (`utils.js`) determines if a resource is "unchanged" using a tiered comparison (cheapest first):
222222

223-
1. If `lastModified` matches cached value AND differs from `indexTimestamp`: unchanged (fast path)
224-
2. If `lastModified` equals `indexTimestamp`: racy-git edge case -- file may have changed during indexing, fall through to integrity check
225-
3. Compare `size` -- if different, changed
226-
4. Compare `integrity` hash -- expensive, last resort
223+
1. Compare `size` -- if different, changed (definite signal; mtime preservation via `cp -p`/`tar -x`/atomic rename does not imply unchanged content)
224+
2. Compare `inode` -- if different, the file was replaced; fall through to integrity check rather than rejecting outright (content may still be identical)
225+
3. If `lastModified` matches cached value AND differs from `indexTimestamp` AND inode matches: unchanged (fast path)
226+
4. If `lastModified` equals `indexTimestamp`: racy-git edge case -- file may have changed during indexing, fall through to integrity check
227+
5. Compare `integrity` hash -- expensive, last resort
227228

228-
Note: inode comparison is defined but currently commented out. `inode` is still stored in TreeNode for future use.
229+
`inode` is optional on both sides (virtual resources, older caches without inode); the inode check is skipped when either side is undefined.
229230

230231
### SharedHashTree and TreeRegistry
231232

.claude/skills/incremental-build/performance-investigation.md

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ UI5_CLI_NO_LOCAL=X UI5_BUILD_NO_WRITE_DEST=X UI5_CACHE_PERF=1 ui5 build --log-le
1818
Flags:
1919
- `UI5_CLI_NO_LOCAL=X` — Use the globally linked CLI (i.e., the development version)
2020
- `UI5_BUILD_NO_WRITE_DEST=X` — Skip writing output to `./dist` (isolates cache/build overhead from disk I/O)
21-
- `UI5_CACHE_PERF=1` — Enable low-level `matchResourceMetadataStrict` counters (see below)
21+
- `UI5_CACHE_PERF=1` — Enable low-level `isResourceUnchanged` counters (see below)
2222
- `--log-level perf` — Show all perf-level log statements
2323

2424
### Three build scenarios
@@ -150,7 +150,7 @@ Per-task index update:
150150

151151
```
152152
perf updateIndices for task 'replaceVersion' of project 'sap.m' resource fetch completed in 3.89 ms: 0 cache hits, 783 cache misses
153-
perf TreeRegistry.flush completed: phase1(removals)=0.00 ms, phase2(upserts)=7.13 ms, phase3(rehash)=9.56 ms | matchMetadataStrictCalls=783, matchMetadataUnchanged=782, modifiedNodesSkips=0
153+
perf TreeRegistry.flush completed: phase1(removals)=0.00 ms, phase2(upserts)=7.13 ms, phase3(rehash)=9.56 ms | isUnchangedCalls=783, isUnchangedHits=782, modifiedNodesSkips=0
154154
perf #flushTreeChanges for task 'replaceVersion' completed in 16.79 ms across 1 registries
155155
perf Updated project indices for task replaceVersion in project sap.m in 21.71 ms
156156
```
@@ -160,7 +160,7 @@ Key metrics:
160160
- **phase1(removals)**: Time removing deleted resources from the Merkle tree.
161161
- **phase2(upserts)**: Time inserting/updating resources in the tree.
162162
- **phase3(rehash)**: Time propagating hash changes up from modified leaves to root. High rehash with few changed resources indicates deep tree structure.
163-
- **matchMetadataStrictCalls / matchMetadataUnchanged**: How many resources were compared, and how many were unchanged (short-circuited by lastModified check). If `calls >> unchanged`, many resources needed content hashing.
163+
- **isUnchangedCalls / isUnchangedHits**: How many resources were compared, and how many were unchanged (short-circuited by lastModified check). If `calls >> hits`, many resources needed content hashing.
164164

165165
Then task execution or skip:
166166

@@ -217,14 +217,15 @@ The four sub-operations (stages, requests, sourceIndex, result) run in parallel
217217

218218
## UI5_CACHE_PERF Counters
219219

220-
Setting `UI5_CACHE_PERF=1` enables low-level counters in `utils.js` for `matchResourceMetadataStrict`:
220+
Setting `UI5_CACHE_PERF=1` enables low-level counters in `utils.js` for `isResourceUnchanged`:
221221

222222
- `calls` — Total invocations
223223
- `shortCircuitTrue` — Fast-path returns (lastModified matches cached and ≠ indexTimestamp → file unchanged, no I/O needed)
224224
- `sizeMismatch` — Changes detected via size check (cheap)
225+
- `inodeMismatch` — File replacement detected via inode change (forces integrity check)
225226
- `integrityFallback` — Full SHA256 content hash required (expensive)
226227

227-
These counters appear in the `TreeRegistry.flush` log as `matchMetadataStrictCalls` and `matchMetadataUnchanged`.
228+
These counters are tallied per-flush in `TreeRegistry.flush` as `isUnchangedCalls` and `isUnchangedHits`.
228229

229230
A high `integrityFallback` count means many files have the same size but different content — this is expensive and may indicate that the `lastModified` short-circuit is not working (e.g., due to timestamp resolution issues).
230231

@@ -303,13 +304,13 @@ These four operations run in parallel. They share I/O bandwidth, so their indivi
303304

304305
**Note:** In CLI mode, cache writes are deferred to the background. The `Wrote build cache` log line appears after `Build succeeded`. The sub-timings tend to be lower (~250-300ms vs ~1400ms) because background writes don't compete with the build for I/O bandwidth.
305306

306-
### 5. matchMetadataUnchanged vs modifiedNodesSkips
307+
### 5. isUnchangedHits vs modifiedNodesSkips
307308

308309
In `TreeRegistry.flush` logs:
309-
- `matchMetadataUnchanged` — Resources whose metadata (lastModified, size, integrity) matched the cached version. No tree modification needed.
310+
- `isUnchangedHits` — Resources whose metadata (lastModified, size, integrity) matched the cached version. No tree modification needed.
310311
- `modifiedNodesSkips` — Tree nodes already marked as modified by a previous flush in the same build. Skipped to avoid redundant work.
311312

312-
When `matchMetadataUnchanged` equals `matchMetadataStrictCalls`, ALL resources were unchanged — the flush was pure overhead.
313+
When `isUnchangedHits` equals `isUnchangedCalls`, ALL resources were unchanged — the flush was pure overhead.
313314

314315
### 6. Dependency index "changed=true" doesn't always mean task re-execution
315316

@@ -367,7 +368,7 @@ When diagnosing slow `writeStageResources`, check the `CAS skipped` vs `CAS writ
367368
4. **Drill down.** For the dominant phase:
368369
- Add more granular `log.perf()` statements if needed
369370
- Use `console.time()`/`console.timeEnd()` for quick local profiling
370-
- Check `matchMetadataStrictCalls` vs `matchMetadataUnchanged` to understand if resources are being unnecessarily re-hashed
371+
- Check `isUnchangedCalls` vs `isUnchangedHits` to understand if resources are being unnecessarily re-hashed
371372

372373
5. **Validate optimization ideas.** When proposing optimizations:
373374
- Consider warm vs stale vs cold cache impact separately

packages/project/lib/build/cache/ProjectBuildCache.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import os from "node:os";
77
import BuildTaskCache from "./BuildTaskCache.js";
88
import StageCache from "./StageCache.js";
99
import ResourceIndex from "./index/ResourceIndex.js";
10-
import {matchResourceMetadataStrict} from "./utils.js";
10+
import {isResourceUnchanged} from "./utils.js";
1111
const log = getLogger("build:cache:ProjectBuildCache");
1212

1313
export class SourceChangedDuringBuildError extends Error {
@@ -991,7 +991,7 @@ export default class ProjectBuildCache {
991991
* Re-reads all source files from disk and compares them against the source index
992992
* to detect whether any source files were modified, added, or deleted during the build.
993993
*
994-
* Uses metadata-only comparison via matchResourceMetadataStrict (skipping tags,
994+
* Uses metadata-only comparison via isResourceUnchanged (skipping tags,
995995
* since tags are build artifacts that always differ from fresh disk reads).
996996
*
997997
* @returns {Promise<boolean>} True if source changes were detected during the build
@@ -1028,7 +1028,7 @@ export default class ProjectBuildCache {
10281028
size: node.size,
10291029
inode: node.inode,
10301030
};
1031-
const isUnchanged = await matchResourceMetadataStrict(
1031+
const isUnchanged = await isResourceUnchanged(
10321032
resource, cachedMetadata, tree.getIndexTimestamp()
10331033
);
10341034
if (!isUnchanged) {

packages/project/lib/build/cache/index/HashTree.js

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import crypto from "node:crypto";
22
import path from "node:path/posix";
33
import TreeNode from "./TreeNode.js";
4-
import {matchResourceMetadataStrict} from "../utils.js";
4+
import {isResourceUnchanged} from "../utils.js";
55

66
/**
77
* @typedef {object} @ui5/project/build/cache/index/HashTree~ResourceMetadata
@@ -388,7 +388,7 @@ export default class HashTree {
388388
const affectedPaths = new Set();
389389

390390
// Phase 1: Categorize each resource. New resources need integrity+size resolved.
391-
// Existing resources are routed through matchResourceMetadataStrict (Phase 2),
391+
// Existing resources are routed through isResourceUnchanged (Phase 2),
392392
// which handles the mtime/size short-circuit with the racy-git protection.
393393
const needsIO = [];
394394

@@ -419,7 +419,7 @@ export default class HashTree {
419419
};
420420
}
421421

422-
// Existing resource with potential change — use matchResourceMetadataStrict
422+
// Existing resource with potential change — use isResourceUnchanged
423423
// for the remaining checks (size comparison, integrity comparison)
424424
const currentMetadata = {
425425
integrity: existingNode.integrity,
@@ -429,13 +429,13 @@ export default class HashTree {
429429
};
430430

431431
const isUnchanged =
432-
await matchResourceMetadataStrict(resource, currentMetadata, this.#indexTimestamp);
432+
await isResourceUnchanged(resource, currentMetadata, this.#indexTimestamp);
433433

434434
if (isUnchanged) {
435435
return {resourcePath, existingNode, isUnchanged: true, tags: resource.getTags()};
436436
}
437437

438-
// Changed — integrity/size are cached in the Resource from matchResourceMetadataStrict
438+
// Changed — integrity/size are cached in the Resource from isResourceUnchanged
439439
const [integrity, size] = await Promise.all([
440440
resource.getIntegrity(),
441441
resource.getSize()

packages/project/lib/build/cache/index/TreeRegistry.js

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import path from "node:path/posix";
22
import TreeNode from "./TreeNode.js";
33
import {tagsEqual} from "./HashTree.js";
4-
import {matchResourceMetadataStrict} from "../utils.js";
4+
import {isResourceUnchanged} from "../utils.js";
55
import {getLogger} from "@ui5/logger";
66
const log = getLogger("build:cache:index:TreeRegistry");
77

@@ -274,8 +274,8 @@ export default class TreeRegistry {
274274
const upsertsByDir = new Map(); // parentPath -> [{resourceName, resource, fullPath, sourceTree}]
275275

276276
const phase2Start = perfEnabled ? performance.now() : 0;
277-
let matchMetadataStrictCalls = 0;
278-
let matchMetadataUnchanged = 0;
277+
let isUnchangedCalls = 0;
278+
let isUnchangedHits = 0;
279279
let modifiedNodesSkips = 0;
280280

281281
for (const [resourcePath, {resource, sourceTree}] of this.pendingUpserts) {
@@ -290,7 +290,7 @@ export default class TreeRegistry {
290290
}
291291

292292
// Pre-resolve I/O for resources that don't exist in any tree (genuinely new).
293-
// For existing resources, matchResourceMetadataStrict's lastModified short-circuit
293+
// For existing resources, isResourceUnchanged's lastModified short-circuit
294294
// avoids I/O in the common case, so those are handled serially in the loop below.
295295
const resolvedNewMetadata = new Map();
296296
const newResourcePaths = [];
@@ -413,15 +413,15 @@ export default class TreeRegistry {
413413
}
414414
} else {
415415
// First time seeing this node — do full comparison
416-
matchMetadataStrictCalls++;
416+
isUnchangedCalls++;
417417
const currentMetadata = {
418418
integrity: resourceNode.integrity,
419419
lastModified: resourceNode.lastModified,
420420
size: resourceNode.size,
421421
inode: resourceNode.inode
422422
};
423423

424-
const isUnchanged = await matchResourceMetadataStrict(
424+
const isUnchanged = await isResourceUnchanged(
425425
upsert.resource,
426426
currentMetadata,
427427
tree.getIndexTimestamp()
@@ -444,7 +444,7 @@ export default class TreeRegistry {
444444
updatedResources.push(upsert.fullPath);
445445
}
446446
} else {
447-
matchMetadataUnchanged++;
447+
isUnchangedHits++;
448448
unchangedNodes.add(resourceNode);
449449
const currentTags =
450450
upsert.resource.getTags?.() ?? upsert.resource.tags ?? null;
@@ -528,8 +528,8 @@ export default class TreeRegistry {
528528
`phase1(removals)=${(phase2Start - phase1Start).toFixed(2)} ms, ` +
529529
`phase2(upserts)=${(phase3Start - phase2Start).toFixed(2)} ms, ` +
530530
`phase3(rehash)=${(now - phase3Start).toFixed(2)} ms | ` +
531-
`matchMetadataStrictCalls=${matchMetadataStrictCalls}, ` +
532-
`matchMetadataUnchanged=${matchMetadataUnchanged}, ` +
531+
`isUnchangedCalls=${isUnchangedCalls}, ` +
532+
`isUnchangedHits=${isUnchangedHits}, ` +
533533
`modifiedNodesSkips=${modifiedNodesSkips}`);
534534
}
535535

packages/project/lib/build/cache/utils.js

Lines changed: 2 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -14,54 +14,7 @@ const perfCounters = {
1414
inodeMismatch: 0,
1515
integrityFallback: 0,
1616
};
17-
export {perfCounters as matchResourceMetadataStrictCounters};
18-
19-
/**
20-
* Compares a resource instance with cached resource metadata
21-
*
22-
* Optimized for quickly rejecting changed files. Performs a series of checks
23-
* starting with the cheapest (timestamp) to more expensive (integrity hash).
24-
*
25-
* @public
26-
* @param {@ui5/fs/Resource} resource Resource instance to compare
27-
* @param {ResourceMetadata} resourceMetadata Resource metadata to compare against
28-
* @param {number} [indexTimestamp] Timestamp of the metadata creation
29-
* @returns {Promise<boolean>} True if resource is found to match the metadata
30-
* @throws {Error} If resource or metadata is undefined
31-
*/
32-
export async function matchResourceMetadata(resource, resourceMetadata, indexTimestamp) {
33-
if (!resource || !resourceMetadata) {
34-
throw new Error("Cannot compare undefined resources or metadata");
35-
}
36-
37-
const currentLastModified = resource.getLastModified();
38-
if (indexTimestamp && currentLastModified > indexTimestamp) {
39-
// Resource modified after index was created, no need for further checks
40-
return false;
41-
}
42-
if (currentLastModified !== resourceMetadata.lastModified) {
43-
return false;
44-
}
45-
if (await resource.getSize() !== resourceMetadata.size) {
46-
return false;
47-
}
48-
const incomingInode = resource.getInode();
49-
if (resourceMetadata.inode !== undefined && incomingInode !== undefined &&
50-
incomingInode !== resourceMetadata.inode) {
51-
return false;
52-
}
53-
54-
if (currentLastModified === indexTimestamp) {
55-
// If the source modification time is equal to index creation time,
56-
// it's possible for a race condition to have occurred where the file was modified
57-
// during index creation without changing its size.
58-
// In this case, we need to perform an integrity check to determine if the file has changed.
59-
if (await resource.getIntegrity() !== resourceMetadata.integrity) {
60-
return false;
61-
}
62-
}
63-
return true;
64-
}
17+
export {perfCounters as isResourceUnchangedCounters};
6518

6619
/**
6720
* Determines if a resource has changed compared to cached metadata
@@ -77,7 +30,7 @@ export async function matchResourceMetadata(resource, resourceMetadata, indexTim
7730
* @returns {Promise<boolean>} True if resource content is unchanged
7831
* @throws {Error} If resource or metadata is undefined
7932
*/
80-
export async function matchResourceMetadataStrict(resource, cachedMetadata, indexTimestamp) {
33+
export async function isResourceUnchanged(resource, cachedMetadata, indexTimestamp) {
8134
if (!resource || !cachedMetadata) {
8235
throw new Error("Cannot compare undefined resources or metadata");
8336
}

packages/project/test/lib/build/cache/index/HashTree.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -264,7 +264,7 @@ test("upsertResources returns empty array when no changes", async (t) => {
264264
t.deepEqual(updated, [], "Should return empty array when no changes");
265265
});
266266

267-
// Companion to the matchResourceMetadataStrict test in cache/utils.js: ensure
267+
// Companion to the isResourceUnchanged test in cache/utils.js: ensure
268268
// upsertResources also detects size mismatches when the cached mtime is preserved.
269269
test(
270270
"upsertResources: classifies as updated when content changed but mtime preserved (size differs)",

0 commit comments

Comments
 (0)