Are cache groups overly restrictive when trying to reuse an existing named chunk?

[scameron]

Hello,

When defining a cache group where the name refers to an existing chunk, there is a validation check done to ensure that the existing chunk is a parent of all selected chunks being considered in the cache group.

The code is here but I'll paste the interesting section for reference:

// Module can only be moved into the existing chunk if the existing chunk
// is a parent of all selected chunks
let isInAllParents = true;
/** @type {Set<ChunkGroup>} */
const queue = new Set();
for (const chunk of selectedChunks) {
    for (const group of chunk.groupsIterable) {
        queue.add(group);
    }
}
for (const group of queue) {
    if (existingChunk.isInGroup(group)) continue;
    let hasParent = false;
    for (const parent of group.parentsIterable) {
        hasParent = true;
        queue.add(parent);
    }
    if (!hasParent) {
        isInAllParents = false;
    }
}

If it finds that the existing chunk is not in the same group as a selected chunk, it then looks at all of the parent groups and it will only succeed if every one of those parent groups contains the existing chunks.

Is this intentional? It seems quite a high bar to say that a chunk must belong to every single parent group. Isn't belonging to one parent enough? If I understand, that should be enough to ensure the existing chunk gets loaded before the selected chunk.

I'm wondering if that continue should actually be a break instead?

[scameron]

It looks like this code was introduced in #11529 by @sokra. Could you comment on the expected behaviour of this change?

Specifically, I'm wondering if it is really necessary that the existing chunk be found in every single ancestor path of every selected chunk? That seems very restrictive (but maybe it is indeed necessary, I don't know).

I have a change that ensures that the existing chunk is just an ancestor of every selected chunk. This assumes that all parent chunks must be loaded before the child chunk. However, I'm not confident that's always true...

Maybe this needs to be refined to do a separate check for each distinct entry point, if a selected chunk has multiple entry points as ancestors?

Would appreciate some guidance here. Thanks!

[nyngwang]

"This assumes that all parent chunks must be loaded before the child chunk." IIRC, the parent-child relationships only exist among chunk groups. On the other hand, it also sounds sub-optimal to ensure that one chunk is loaded because most of the time you might eventually load a lot of chunks (from parent chunk groups) of your projects.

[nyngwang]

This is just my guess:

The purpose of splitting a new chunk is to de-duplicate the same chunk of code. But for every chunk in selectedChunks, you need to make sure that things can still work after the de-duplication. In the section of code you quoted, we have found a chunk existingChunk which has a name that fits one cacheGroup schema, but we have to check what I have said.

This part is my guessing:

To do so we have to make sure that existingChunk is in every chunkGroup for all selectedChunks, because we don't want any other dependent chunkGroups to misfunction after the de-duplication.

On the other hand, given that a chunk can be in many chunkGroups, we don't know which chunkGroup will be chosen for the final state (I guess that only one chunkGroup will be used for every chunk). To cover all cases, every chunkGroup of every selectedChunks is added into the queue. By the same reasoning, we have to add all parents of a chunkGroup if existingChunk is missing from it, because we don't know which chunkGroup will be used to cover the current chunk.

To conclude,

1. If "Isn't belonging to one parent enough?" is correct, then line 7 in the quoted snippet, i.e. for (const group of chunk.groupsIterable) {, also needs to be fixed, because it adds all chunkGroup for every selectedChunks.
2. "I'm wondering if that continue should actually be a break instead?" might not be a solution, since you might skip all chunkGroups from some selectedChunks.

[scameron]

All of this hinges on the question about the meaning of the parent-child relationship in chunk groups. Is it true that all parent chunk groups must be loaded before the child chunk group is loaded? Or is it possible that only parent chunk group can be loaded before the child chunk group?

I have a feeling it's the actually the latter and my original assumption was wrong. I've never seen this written down anymore, but thinking about it more, it does make some sense. In that case, my idea doesn't work at all.

[nyngwang]

It's a great idea to think about whether something is necessary to optimize an existing tool further. But I think now the author of webpack is busy building something else so I'm afraid that we are the ones who need to test the idea ourselves. Currently, I don't have time to dive into the source code.