Skip to content

perf: precompute directory update timestamps - #4479

Closed
fabricinhopremium-code wants to merge 1 commit into
github:marketplacefrom
fabricinhopremium-code:perf/directory-last-updated-index
Closed

fabricinhopremium-code wants to merge 1 commit into
github:marketplacefrom
fabricinhopremium-code:perf/directory-last-updated-index

Conversation

@fabricinhopremium-code

Copy link
Copy Markdown

$## Summary

This change avoids repeatedly scanning the full git-date map for each plugin and canvas item when populating lastUpdated metadata.

  • build a directory-level timestamp index once
  • resolve directory updates in O(1) during generation
  • preserve the existing output while reducing repeated traversal work during website-data generation

Validation

  • & C:\\Users\\fabricio.fmc\\AppData\\Local\\Programs\\Qlik\\Sense\\Node\\node.exe --check eng\\generate-website-data.mjs

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

🚦 Submission status: ⏳ Awaiting automation

Risk tier: merge-risk:high — Privileged execution, automation, or review-policy change
Required to merge: passing submission-gate checks plus 2 approvals from reviewers with write access, including a maintainer with admin or maintain permission.

Why this tier
  • eng/generate-website-data.mjs is a high-risk path (automation, scripts, MCP config, hooks, or review policy)

Automated checks

Check Status Details
Contributor reputation 🔧 Infrastructure failure Waiting for a maintainer to approve workflow runs for this PR · logs
Duplicate resource scan ⚠️ Failed (advisory, non-blocking) Waiting for a maintainer to approve workflow runs for this PR · logs
PR quality signal ⚠️ Failed (advisory, non-blocking) Waiting for a maintainer to approve workflow runs for this PR · logs

Action needed

  • 🔧 Contributor reputation hit an automation problem that is not caused by your contribution. Comment /rerun-checks to retry; maintainers are notified if it keeps failing.
  • ⚠️ Advisory checks did not complete (Duplicate resource scan, PR quality signal). This does not block the PR.

Review

  • Approvals: 0/2
  • Assigned reviewer: not assigned yet — comment /request-review to ask for one
  • Review target date: not set
  • Still needed: 2 more approval(s); an approval from a maintainer with admin or maintain permission
  • The core-maintainers pool is not staffed yet; an approver with admin or maintain permission is required instead.

Commands

Command Who What it does
/rerun-checks PR author, maintainers Re-runs failed or incomplete checks and re-evaluates this gate
/request-review PR author, maintainers Asks the review rotation to assign a reviewer (adds needs-reviewer)

Updated for fcefc64 · gate run · This comment is maintained automatically — see submission gate docs.

@aaronpowell

Copy link
Copy Markdown
Contributor

/rerun-checks

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

🔁 /rerun-checks for fcefc64

No failed or incomplete checks to re-run.

  • Label PR Intent waiting for a maintainer to approve workflow runs.
  • PR Duplicate Check waiting for a maintainer to approve workflow runs.
  • Contributor Reputation Check waiting for a maintainer to approve workflow runs.
  • PR Quality Signal Review waiting for a maintainer to approve workflow runs.
  • Submission Gate waiting for a maintainer to approve workflow runs.

The status comment updates when the checks finish.

@aaronpowell

aaronpowell commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Thanks for the contribution, but after benchmarking this against the repo's actual scale (2,357 tracked files, 73 directories queried across plugins/extensions), the precomputed index approach is not actually faster — it's slightly slower (~5.5ms/run vs ~3.5ms/run for the original).

Why: the original code's startsWith check is a cheap early-exit per file, with Date.parse only running on matches. The new approach must scan every file on every build call (even directories never queried), splitting paths and doing multiple Map set/get operations with object allocation per path segment — more total work than it saves at this file/directory ratio. It's also rebuilt from scratch in two separate call sites (generatePluginsData and generateCanvasManifest), so the index itself is never reused across the one place it could help.

While the new code is algorithmically O(files × depth + dirs) vs the original's O(dirs × files), the constant-factor overhead outweighs the savings here since neither files nor dirs is large enough for the asymptotic improvement to matter. This reads as a premature optimization without profiling data showing a real bottleneck, so I'm closing it. Happy to revisit if there's a measured regression this addresses.

@aaronpowell aaronpowell closed this Oct 7, 2026
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.

2 participants