perf: detach children inline in Empty()#580
Conversation
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.
|
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. |
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.