Optimize for large bundles with numerous globs by snipervld · Pull Request #326 · ligershark/WebOptimizer

snipervld · 2024-09-25T20:13:01Z

Update 2026-06-11: I've replaced manual caching with GlobbingUrlBuilder. Plus, I've added two more tests to check that GlobbingUrlBuilder works with nested directories/files.

At the moment, Asset.ExpandGlobs{:.c#} doesn't cache results of provider.GetAllFiles("/"), so in the following example (simplified example - in the real app, this bundle contains different globs at different nested levels):

builder
    .Services
    .AddWebOptimizer(
        pipeline =>
        {
            //...
            pipeline.AddJavaScriptBundle(
                "bundle1",
                "js/*",
                "js/*",
                "js/*",
                "js/*",
                "js/*",
                "js/*");
            //...
        },
        options =>
        {
            options.EnableTagHelperBundling = false;
        });

Asset.ExpandGlobs will get all files 6 times. It can be a problem, if the virtual files hierarchy is huge.

This PR caches the results of provider.GetAllFiles("/") per provider into HttpContext.Items dictionary.

Also, this fixes the case when many complex bundles are located on the same page, e.g.:

...
<script src="~/bundle1"></script>
<script src="~/bundle2"></script>
<script src="~/bundle3"></script>
...

Each of them will reuse the cached file hierarchy if possible.

P.S. As you can see, I've tested this with EnableTagHelperBundling = false. ScriptTagHelper and LinkTagHelper call Asset.ExpandGlobs on each execution, which resulted in 5 seconds of rendering for just the script tags in my app, instead of something like ~0.1 seconds. :)

jzabroski · 2025-06-07T11:37:10Z

This should be re-worked to use GlobbingUrlBuilder - It internally uses a Cache and is backed by the full faith, credit and trust of ASP.NET Core :D

jzabroski · 2026-06-11T18:43:08Z

I'll give it a whirl and let @madskristensen know but superficially makes sense.

There's another issue in the Open Issues list where I looked into a somewhat related issue with file provider use cases.

I personally think this code base needs a regression suite around that stuff. Might re-clone the repo and ask Claude to help think through OATS pair-wise tests of feature combinations.

snipervld · 2026-06-11T20:57:23Z

@jzabroski, found the bug: GlobbingUrlBuilder doesn't keep the original included files order,
e.g. source files js/a.js,js/b.js,js/c.js... may turn into js/x.js,js/c.js,...

I'll restore the foreach loop and call the builder inside it. Of course, I'll add tests for this :)

madskristensen · 2026-06-11T21:48:59Z

just let me know when this is ready to merge

snipervld · 2026-06-12T01:03:03Z

@jzabroski, @madskristensen, done

jzabroski · 2026-06-12T02:39:39Z

GlobbingUrlBuilder doesn't keep the original included files order, e.g. source files js/a.js,js/b.js,js/c.js... may turn into js/x.js,js/c.js,...

@snipervld Thanks for putting this together; I didn't see where your tests explicitly ensured this original order is preserved? Otherwise, I added a minor comment about the MockFileInfo private constructor. I think the first parameter name should be directoryName rather than name, to reflect the fact it is just a helper for the CreateDirectory factory method.

snipervld · 2026-06-12T10:02:46Z

@jzabroski

I didn't see where your tests explicitly ensured this original order is preserved?

Oops, sorry, I fixed the logic, but forgot about them after the comment :)

Add some more tests.

@jzabroski, I didn't see your comments about MockFileInfo (maybe you didn't Submit Review?), but I think I got your idea: renamed name into directoryName/fileName

Used GlobbingUrlBuilder to find matching files in the file provider. Added tests for nested file structure (there were tests just for plain FSs). Caveat: GlobbingUrlBuilder uses binary search, so the order of files matching the glob pattern may differ from the previous implementation. E.g. if the bundle is defined this way: pipeline.AddJavaScriptBundle("bundle1", "js/a.js", "js/x-*.js", "js/b.js", "js/c/js"); files within "js/x-*.js" group may have the different order.

jzabroski · 2026-06-12T11:56:41Z

Yes, there seems to be a weird glitch where I can't submit the review since im technically not marked as a reviewer.

jzabroski · 2026-06-12T11:58:01Z

On second thought...it might be because I added this comment a year ago 🤔

snipervld · 2026-06-12T13:09:27Z

@jzabroski, what do you think about 2d5d828?

jzabroski

@snipervld There's a few code clean-ups I recommend doing to make sure as other contributors build on top of your tests, they do not violate invariants in your test setups. Or, if they do, future PR reviews are easier to distinguish the changes and think critically about.

jzabroski · 2025-05-19T18:38:57Z

            context.Setup(c => c.RequestServices.GetService(typeof(IMemoryCache)))
                .Returns(cache.Object);
            context.SetupGet(c => c.Request.PathBase).Returns(pathBase);
+            context.SetupGet(c => c.Items).Returns(new Dictionary<object, object>());


You should call Mock.VerifyAll(context, optionsFactory, optionsMonitorCache, cache, asset, assetPipeline); at the end of the method, where you perform Asserts.

jzabroski · 2026-06-12T02:35:30Z

            LastModified = lastModified;
        }

+        private MockFileInfo(string name, DateTimeOffset lastModified, IList<MockFileInfo> files = null)


Rename the first parameter to directoryName; this makes it more intuitive that this private constructor is meant to serve as a back-end constructor for the CreateDirectory factory method.

jzabroski · 2026-06-12T14:27:22Z

+            context.Verify(c => c.RequestServices.GetService(typeof(IWebHostEnvironment)));
+            context.Verify(c => c.RequestServices.GetService(typeof(IMemoryCache)));
+            context.Verify(c => c.Response.Headers);
+            env.Verify(e => e.WebRootFileProvider);


Why not use Mock.VerifyAll?

I tried Mock.VerifyAll, but it failed, because Mock.VerifyAll required all mock file provider's setup to be executed.

In other tests, where Mock.VerifyAll worked, I used it.

Ok, I'll recheck tests once again.

jzabroski · 2026-06-12T14:27:34Z

+            context.Verify(c => c.RequestServices.GetService(typeof(IWebHostEnvironment)));
+            context.Verify(c => c.RequestServices.GetService(typeof(IMemoryCache)));
+            context.Verify(c => c.Response.Headers);
+            env.Verify(e => e.WebRootFileProvider);


Why not use Mock.VerifyAll?

jzabroski · 2026-06-12T14:27:45Z

+            context.Verify(c => c.RequestServices.GetService(typeof(IWebHostEnvironment)));
+            context.Verify(c => c.RequestServices.GetService(typeof(IMemoryCache)));
+            context.Verify(c => c.Response.Headers);
+            env.Verify(e => e.WebRootFileProvider);


Why not use Mock.VerifyAll?

jzabroski · 2026-06-12T14:28:00Z


            Assert.Equal(new[] { "b.css", "a.css" }, files);
+
+            env.Verify(e => e.WebRootFileProvider);


Mock.VerifyAll

jzabroski · 2026-06-12T14:28:09Z


            Assert.Equal(new[] { "sub/nested.css", "root.css" }, files);
+
+            env.Verify(e => e.WebRootFileProvider);


Mock.VerifyAll

jzabroski · 2026-06-12T14:28:18Z

            Assert.True(indexStyleA < indexAlpha && indexStyleB < indexAlpha,
                "style-*.css group precedes alpha.css group");
+
+            env.Verify(e => e.WebRootFileProvider);


Mock.VerifyAll

Replaced Moq-based MockFileProvider with a plain implementation.

snipervld · 2026-06-13T22:13:05Z

@jzabroski, replaced manual verifies with VerifyAll.

jzabroski · 2026-06-14T15:17:11Z

LGTM. Thank you for taking the feedback in so fast.

snipervld · 2026-06-14T15:19:23Z

@madskristensen,

just let me know when this is ready to merge

ready to merge.

snipervld force-pushed the master branch from 6b805dd to 32441ec Compare June 11, 2026 13:00

snipervld force-pushed the master branch 2 times, most recently from ceb456b to ce5d20d Compare June 12, 2026 01:02

snipervld added a commit to snipervld/WebOptimizer that referenced this pull request Jun 12, 2026

ligershark#326 add tests for source order

975464d

snipervld added 2 commits June 12, 2026 13:05

ligershark#326 add tests for source order

86f5442

snipervld force-pushed the master branch from 975464d to 86f5442 Compare June 12, 2026 10:05

snipervld mentioned this pull request Jun 12, 2026

Allow to use inline tag helpers without physical provider #384

Open

snipervld added a commit to snipervld/WebOptimizer that referenced this pull request Jun 12, 2026

ligershark#326 tighten mock verification in tests

2d5d828

jzabroski suggested changes Jun 12, 2026

View reviewed changes

ligershark#326 tighten mock verification in tests

b462e49

Replaced Moq-based MockFileProvider with a plain implementation.

snipervld force-pushed the master branch from 2d5d828 to b462e49 Compare June 13, 2026 22:11

snipervld requested a review from jzabroski June 13, 2026 22:13

jzabroski approved these changes Jun 14, 2026

View reviewed changes


		Assert.Equal(new[] { "b.css", "a.css" }, files);

		env.Verify(e => e.WebRootFileProvider);


		Assert.Equal(new[] { "sub/nested.css", "root.css" }, files);

		env.Verify(e => e.WebRootFileProvider);

Uh oh!

Conversation

snipervld commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jzabroski commented Jun 7, 2025

Uh oh!

jzabroski commented Jun 11, 2026

Uh oh!

snipervld commented Jun 11, 2026

Uh oh!

madskristensen commented Jun 11, 2026

Uh oh!

snipervld commented Jun 12, 2026

Uh oh!

jzabroski commented Jun 12, 2026

Uh oh!

snipervld commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jzabroski commented Jun 12, 2026

Uh oh!

jzabroski commented Jun 12, 2026

Uh oh!

snipervld commented Jun 12, 2026

Uh oh!

jzabroski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snipervld commented Jun 13, 2026

Uh oh!

jzabroski commented Jun 14, 2026

Uh oh!

snipervld commented Jun 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

snipervld commented Sep 25, 2024 •

edited

Loading

snipervld commented Jun 12, 2026 •

edited

Loading