Skip to content

perf: detach children inline in Empty()#580

Open
jvoisin wants to merge 1 commit into
PuerkitoBio:masterfrom
jvoisin:empty
Open

perf: detach children inline in Empty()#580
jvoisin wants to merge 1 commit into
PuerkitoBio:masterfrom
jvoisin:empty

Conversation

@jvoisin

@jvoisin jvoisin commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Empty() called n.RemoveChild(c) in a loop where c was always the FirstChild. RemoveChild's general logic is overkill for that case: the panic-check on c.Parent is always true, the PrevSibling branch is always nil, the FirstChild reassignment is overwritten on every iteration, and the LastChild check only ever matches on the final child.

Walk the sibling chain once via NextSibling, nil out each detached child's parent/sibling pointers, and clear n.FirstChild / n.LastChild exactly once at the end.

Empty() called n.RemoveChild(c) in a loop where c was always the
FirstChild. RemoveChild's general logic is overkill for that case:
the panic-check on c.Parent is always true, the PrevSibling branch
is always nil, the FirstChild reassignment is overwritten on every
iteration, and the LastChild check only ever matches on the final
child.

Walk the sibling chain once via NextSibling, nil out each detached
child's parent/sibling pointers, and clear n.FirstChild / n.LastChild
exactly once at the end.
@mna

mna commented Jun 4, 2026

Copy link
Copy Markdown
Member

So I have a growing malaise with those perf PRs, in that they consistently and increasingly break a nice property of the codebase where there's a small number of core functions that are reused and composed to build a large API surface. A very common approach to those performance improvements are to inline some of that code reuse and logic in order to remove the parts that are unnecessary. Of course it results in more efficient runtime and less allocations, but very importantly, it adds complexity, code and logic duplication and makes the project harder to maintain (changing one thing here means remembering to tweak it there and there and ...).

I'm gonna pause merging those types of PRs for now, I want to think about them a bit more and may close them eventually if I'm not comfortable with the change. I appreciate the dedication to extract top performance, but it's important to put this package in its typical context, which is often in web-based and/or database-backed applications, where milliseconds and megabytes are usual metrics. The package has been very stable for a long time and I want to ensure it stays that way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants