Repository navigation
Conversation
|
Damn, didn't thought copilot review would start automatically on another repo beside mines |
There was a problem hiding this comment.
Pull request overview
This PR adds an incremental output mode to the output contrib plugin to avoid wiping and rewriting entire pack directories on every build, addressing performance issues for large projects (issue #512).
Changes:
- Add
incremental: booltoOutputOptions(defaultFalse). - Implement
incremental_save()to delete removed files, prune empty directories, and skip writing unchanged files. - Update
output()to use incremental saving for non-zipped packs when enabled.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
should meta.output.incremental option be true by default? |
Honestly, yes imo. it's an improvement. |
I was just on a call with airdox, and he says it's not worth it on linux but windows would be good. |
|
AirDox just tested from my branch on linux:
|
rx-dev
left a comment
rx-dev
left a comment
There was a problem hiding this comment.
Some things were nitpicks, i realize the original code here used os than I realized though I really do think the pathlib apis are a bit cleaner and has better patterns which would cleanup more of the use-cases here.
There's a lot of functions written out here. Some is good organization, but some feels like over-doing it. Though, each one is described well so perhaps it would be good for future maintenance.
| if incremental is None: | ||
| meta_opts = ctx.meta.get("output") | ||
| if isinstance(meta_opts, dict): | ||
| incremental = bool(meta_opts.get("incremental", False)) |
There was a problem hiding this comment.
hmmm, i don't think u should be reading ctx.meta here, the way that @configurable works in the first place should auto-fill the opts with stuff from meta.
There was a problem hiding this comment.
@configurable does auto-fill from meta, but only when the plugin is called with no kwargs.
The toolchain registers this one as output(directory=ctx.output_directory) at project.py (line 315), and ctx.validate takes options=kwargs or None, so the meta branch at context.py (line 274) is skipped for every normal build.
It's only droppable if we make ctx.validate merge meta under the explicit kwargs, but since this changes the priority for each configurable plugin, it would be better to create a separate pull request.
|
Applied most of the review comments. Bench for the pathlib vs os: git clone https://github.com/Paralya/Switch
py bench_scan.py "Switch"Log of my execution PS D:\advanced_desktop\beet\bench> py .\bench_scan.py "D:\advanced_desktop\Switch"
D:\advanced_desktop\Switch (9969 files, 15 repeats)
scan_directory
os.scandir min 93.02 ms median 95.53 ms
os.walk + os.stat min 765.60 ms median 815.27 ms
Path.walk min 973.40 ms median 998.25 ms
Path.rglob min 1789.88 ms median 1882.13 ms
prune_empty_directories
os.walk + os.rmdir min 230.64 ms median 260.20 ms
Path.walk + Path.rmdir min 249.36 ms median 273.12 ms |
|
@rx-dev Any news on that one? |
…beet's incremental output beet's output plugin now saves a folder through `incremental_save` instead of `Pack.save` (mcbeet/beet#513), which the image bytes reuse was hooked on. A generated image encoded differently on another machine would then be seen as changed and rewritten. `incremental_save` is wrapped the same way when present.
* fix(merge_smithed_weld): 🐛 Keep the merged archive of a pack type welded later in the pipeline With the split entry points, the first one dropped the `_with_libs.zip` of the pack type the second one welds, so that one never found its archive again: the merge always ran in full instead of being skipped when its sources were unchanged (about 2s on every build of a large resource pack). The archive is now moved to the weld cache instead of deleted, still out of the output directory, and taken back by the entry point welding it. What nobody took back is deleted once the pipeline ran. * perf(archive): ⚡ Reuse the unchanged entries of the previous archive Deflate gives the same bytes for the same input, so an entry holding exactly what the archive being replaced holds is now copied still compressed instead of being compressed again. The previous entry is decompressed and compared whole, a matching checksum alone is not trusted. The weld writes through the same class and benefits too. Generated images now take their bytes from the previous archive when their pixels did not change. The previous lookup went through the output folder named after `pack.name`, which during the pipeline is still the loaded folder (`src`), so it only worked for projects whose source folder was named like the output. * perf(lang_file): ⚡ Find text components 11 times faster and cache their split `TEXT_RE` started with an optional quote and read values one character at a time, so the engine tried a match at every position of every file. It now starts with the literal `text` and reads values with a greedy class, the key's opening quote being checked by `extract_texts`. Same matches on 7666 files of the projects and 300k random strings. `lang_parts` is pure and called on the same few texts over and over (88735 calls for 1678 texts in MC Guns System), so it is cached for the build. * perf(ingame_manual): ⚡ Skip drawing and encoding the images that did not change Every build decoded, resized and encoded again each manual image in the font cache. `save_png` now skips the encoding when the file holds what it wrote from the same pixels, and the item and wiki icons are skipped before even opening their render when `is_png_recorded` finds them drawn from the same source file, parameters and StewBeet version. Both check the file's size and modification time, so anything that touched it gets it written again. * perf(ingame_manual): ⚡ Skip redrawing an unchanged showcase image The showcase images are drawn in a background process the build waits for before exiting: several seconds for a grid of every item. An image is now redrawn only when its items, their renders or the case template changed since the one on disk was drawn. * perf(sniffer): ⚡ Keep the write calls of unchanged source files between builds Parsing and walking every project source file was nearly all of the sniffer's time during the project's writes. Each file's write calls are now kept in the beet cache with its size and modification time, and parsed again only when it changed or when the way write calls are recognized changes. * perf(core): ⚡ Import mecha only where it is used `core.source_paths` imported mecha for a type annotation, loading it on every `import stewbeet` even for projects without mecha in their pipeline. * fix(initialize): 🐛 Keep the bytes of unchanged generated images with beet's incremental output beet's output plugin now saves a folder through `incremental_save` instead of `Pack.save` (mcbeet/beet#513), which the image bytes reuse was hooked on. A generated image encoded differently on another machine would then be seen as changed and rewritten. `incremental_save` is wrapped the same way when present.
Fixes #512
A new incremental option (default to false) can be given to the output plugin.
either OutputOptions or in beet.yml:
When enabled:
File watchers will now be less in pain each build.
On my tests, I get faster build time, idk if you want to make it True by default so I put at False.
Bonus: Optimized the list_files() function that was building a Path to compute relative time for each file:
8f4d701