Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a PathBuilder #850

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Conversation

gunnarmorling
Copy link
Member

Hey @gsmet, this is a small experiment based on your perf improvement work. You pointed out that the allocation of nodes and paths is a problem and I agree.

We went for the immutable approach back in the day as we often had issues with paths which were referenced e.g. in a constraint violation and then be altered later on in the course of subsequent validation of other constraints. I think the best way forward would be to separate mutable and immutable path and node types. The engine would in general work with the mutable representations and only "freeze out" an immutable reference when needed (constraint violation creation, storing processed path).

As a quick attempt, this PR is doing this very hackishly for paths, could you check whether it already makes a noticable difference? I'd expect a bigger difference when doing the same for nodes, but an early check may be useful to see whether it's in the right direction or not.

gsmet and others added 21 commits September 18, 2017 09:09
We can directly set the element in the list. It avoids some list
resizing.
only have the default group

Also optimize a bit the advanced case with groups.
default violation

Also optimize a bit the concatenation of the default violation and the
custom ones to avoid creating a list too small and resize it each time.
A lot of NodeImpls are created during the build of the path and some of
them are simply discarded as they are "modified" (NodeImpl are immutable
so we create a new) further away. Thus we better avoid building the
hashCode each time.
Otherwise, we "compare" the whole path several times while comparing a
path.
It avoids copying the list when not strictly necessary
It avoids a lot of initialization/resizing and in the end it's more
efficient.
So we don't need to do it twice. Note that this change uncovers the fact
that in ConstraintValidatorContext, calling atKey() or atIndex() makes
the node iterable.

It was already the case before and I think it's acceptable.

It brings its own performance improvements as it avoids initializing 1
NodeImpl and creating 1 copy of the Path.
list with one element

It doesn't seem necessary to consider more elements as the list will be
copied when new nodes will be added.
Only property and container node exposes it to users.
@gunnarmorling
Copy link
Member Author

Hey @gsmet, what should we do about this one? Would you like to pursue the mutable/immutable split for paths and nodes at this time? If not, you could at least integrate my first commit and we should be good to go once you've addressed that one remark about a hash code.

On the split above, instead of using Stack for mutable paths, we also may consider to make this a linked list of dedicated node implementations which should help with reducing memory allocation.

@gsmet gsmet changed the title HV-1480 Introduce a PathBuilder Oct 4, 2017
Base automatically changed from master to main March 19, 2021 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants