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

mutation: Use chunk-based mutation approach for Map #713

Merged
merged 3 commits into from
Apr 20, 2023
Merged

Conversation

fmeum
Copy link
Contributor

@fmeum fmeum commented Apr 18, 2023

The previous Map mutator performed both single element as well as chunk
mutations, which isn't necessary now that we can bias chunks towards
being small.

The old implementation didn't check that initInPlace and read obeyed
the specified minSize. Since non-zero minSizes can be problematic in
read, they are not supported for now.

There were also a few issues with key mutations: Instead of retrying key
creation separately for each key, which can result in a large amount of
wasted attempts if the key set is almost saturated, all such attempts
now share a single failure counter. Keys are now properly detached
before being mutated, which is necessary if map keys are mutable.

The fallback to value mutation if key mutation failed also didn't
behave correctly: It called setValue on a newly allocated Map.Entry,
which is a noop.

@fmeum fmeum requested review from a team and bertschneider April 18, 2023 07:39
The mutator tended to generate arrays of length almost 1000 very
quickly, which is unnecessary in tests and also doesn't match the actual
behavior at runtime.
Makes it possible to run the tests without building proto-related code.
@fmeum fmeum force-pushed the FUZZ-659-map-fixes branch 2 times, most recently from cdf2fcc to 253c26a Compare April 18, 2023 09:22
@fmeum
Copy link
Contributor Author

fmeum commented Apr 18, 2023

I submitted bazel-contrib/rules_jvm#175 to get better logs and noticed that there is an OOM issue that I will need to investigate.

@fmeum fmeum force-pushed the FUZZ-659-map-fixes branch 2 times, most recently from 93940bd to 93b60ee Compare April 19, 2023 20:18
Copy link
Contributor

@bertschneider bertschneider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

The previous Map mutator performed both single element as well as chunk
mutations, which isn't necessary now that we can bias chunks towards
being small.

The old implementation didn't check that `initInPlace` and `read` obeyed
the specified `minSize`. Since non-zero `minSize`s can be problematic in
`read`, they are not supported for now.

There were also a few issues with key mutations: Instead of retrying key
creation separately for each key, which can result in a large amount of
wasted attempts if the key set is almost saturated, all such attempts
now share a single failure counter. Keys are now properly detached
before being mutated, which is necessary if map keys are mutable.

The fallback to value mutation if key mutation failed also didn't
behave correctly: It called `setValue` on a newly allocated `Map.Entry`,
which is a noop.

The number of iterations in the StressTest is halved to prevent OOMs on
the runners.
@fmeum fmeum enabled auto-merge (rebase) April 20, 2023 12:40
@fmeum fmeum merged commit 0554df8 into main Apr 20, 2023
@fmeum fmeum deleted the FUZZ-659-map-fixes branch April 20, 2023 12:50
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