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.

Sign up for GitHub

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

Jump to bottom

resurrect the GC / memory pressure test suite #1603

Closed
flavorjones opened this issue Feb 9, 2017 · 2 comments · Fixed by #3050
Closed

resurrect the GC / memory pressure test suite #1603

flavorjones opened this issue Feb 9, 2017 · 2 comments · Fixed by #3050
Labels
topic/ci topic/memory Segfaults, memory leaks, valgrind testing, etc.

Comments

@flavorjones
Copy link
Member

flavorjones commented Feb 9, 2017 •
edited
Loading

The file test/test_memory_leak.rb and the teardown behavior controlled by the NOKOGIRI_GC env var could be useful.

Consider resurrecting these.

Acceptance criteria:

  • suite needs to finish in a reasonable amount of time (currently many are looping literally forever)
  • suite needs to be runnable under valgrind (so, not a separate rake task, but possibly controlled by an environment variable for use with the test:valgrind task from hoe-debugging)
  • tests must clearly pass or fail. previously I'd just run a single test and watch the process heap size to see if it increased. It would be great to run under valgrind and use those metrics for something.
  • suite should be run in the main concourse pipeline
@flavorjones flavorjones mentioned this issue Jan 5, 2019
Closed
@flavorjones flavorjones added the topic/memory Segfaults, memory leaks, valgrind testing, etc. label Feb 2, 2020
@sparklemotion sparklemotion deleted a comment from rubarb666 Oct 20, 2020
@flavorjones flavorjones mentioned this issue Nov 1, 2020
Closed
@flavorjones flavorjones mentioned this issue Nov 29, 2020
Closed
@flavorjones flavorjones mentioned this issue Oct 25, 2021
Closed
flavorjones added a commit that referenced this issue Oct 15, 2022
@flavorjones
test: small improvements to memory leak test suite
8dacf06 
Related to #1603
@flavorjones
Copy link
Member Author

After a little bit of work in #2658 I'm forming some more concrete opinions on this.

I think we could make this a runnable suite if we use:

  • the value returned from MemInfo.rss (introduced in af99c52)
  • and Minitest::BenchSpec (introduced in 46dcc60)

to ensure that memory usage isn't increasing after some number of iterations.

flavorjones added a commit that referenced this issue Oct 15, 2022
@flavorjones
test: small improvements to memory leak test suite
e1dd979 
Related to #1603
flavorjones added a commit that referenced this issue Oct 16, 2022
@flavorjones
test: small improvements to memory leak test suite
65a28e3 
Related to #1603
@flavorjones flavorjones mentioned this issue Nov 20, 2023
Merged
flavorjones added a commit that referenced this issue Nov 21, 2023
@flavorjones
fix: gumbo memory leak on abandoned tags (#3036)
fd9f9e1 
**What problem is this PR intended to solve?**

The fuzzer in #3007 found a memory leak when incomplete tags are
encountered.

The repro added to the memory leak test suite is:

```html
<asdfasdfasdfasdfasdfasdfasdfasdfasdfasdf foo="bar
```

which will leak the tag name without this fix.

cc @stevecheckoway 

**Have you included adequate test coverage?**

Yes, but added it to the memory leak suite which can only be run
manually today (see #1603 for context on plans to improve it).

**Does this change affect the behavior of either the C or the Java
implementations?**

No functional change.
@flavorjones flavorjones mentioned this issue Dec 11, 2023
Merged
@flavorjones
Copy link
Member Author

ZOMG see #3050

flavorjones added a commit that referenced this issue Dec 11, 2023
@flavorjones
ci: resurrect the memory test suite (#3050)
b0bbd3e 
**What problem is this PR intended to solve?**

Closes #1603

I don't know that all of the memory suite tests are valuable; but
running those tests in CI shows that I can add new tests for suspected
memory leaks (since we now have OSS-Fuzz reports being generated thanks
to @fuzzy-boiii23a).

Add some test helpers (which only work on Linux):
- `meminfo_page_size`
- `meminfo_vmsize`
- `meminfo_rss`
- `memwatch` test helper
  - under valgrind/ruby_memcheck, it simply loops
  - otherwise tries to detect leaks by watching vmsize

Rename test/test_memory_leak.rb to test/test_memory_usage.rb:
- update to minispec style and do other cleanup
- move some memory tests from elsewhere in the suite to this file
- use the memwatch helper

Passing tests look like:

```
memwatch: test_0001_test_jumping_sax_handler: (n=1        0) 720496 Kb
memwatch: test_0001_test_jumping_sax_handler: (n=2   147800) 724880 Kb, Δ +4384
memwatch: test_0001_test_jumping_sax_handler: (n=3   295600) 720912 Kb, Δ -3968
memwatch: test_0001_test_jumping_sax_handler: (n=4   443400) 721208 Kb, Δ +296
memwatch: test_0001_test_jumping_sax_handler: (n=5   591200) 720496 Kb, Δ -712
memwatch: test_0001_test_jumping_sax_handler: (n=6   739000) 720496 Kb, Δ +0
memwatch: test_0001_test_jumping_sax_handler: (n=7   886800) 732456 Kb, Δ +11960
memwatch: test_0001_test_jumping_sax_handler: (n=8  1034600) 720496 Kb, Δ -11960
memwatch: test_0001_test_jumping_sax_handler: (n=9  1182400) 720496 Kb, Δ +0
memwatch: test_0001_test_jumping_sax_handler: (n=10 1330200) 731124 Kb, Δ +10628
memwatch: test_0001_test_jumping_sax_handler: (n=11 1478000) 732544 Kb, Δ +1420
memwatch: test_0001_test_jumping_sax_handler: elapsed 8.888018s
memwatch: test_0001_test_jumping_sax_handler: slope = 5.95228 (r^2 = 0.296)

memwatch: test_0001_test_leak_on_node_replace: (n=1        0) 828140 Kb
memwatch: test_0001_test_leak_on_node_replace: (n=2    67100) 828416 Kb, Δ +276
memwatch: test_0001_test_leak_on_node_replace: (n=3   134200) 828416 Kb, Δ +0
memwatch: test_0001_test_leak_on_node_replace: (n=4   201300) 828416 Kb, Δ +0
memwatch: test_0001_test_leak_on_node_replace: (n=5   268400) 828416 Kb, Δ +0
memwatch: test_0001_test_leak_on_node_replace: (n=6   335500) 828416 Kb, Δ +0
memwatch: test_0001_test_leak_on_node_replace: (n=7   402600) 828416 Kb, Δ +0
memwatch: test_0001_test_leak_on_node_replace: (n=8   469700) 828416 Kb, Δ +0
memwatch: test_0001_test_leak_on_node_replace: (n=9   536800) 828416 Kb, Δ +0
memwatch: test_0001_test_leak_on_node_replace: (n=10  603900) 828416 Kb, Δ +0
memwatch: test_0001_test_leak_on_node_replace: (n=11  671000) 828416 Kb, Δ +0
memwatch: test_0001_test_leak_on_node_replace: elapsed 10.268576s
memwatch: test_0001_test_leak_on_node_replace: slope = 0.19145 (r^2 = 0.250)
```

Failing tests look like:

```
memwatch: test_0012_#2114 RelaxNG schema parsing: (n=1        0) 758452 Kb
memwatch: test_0012_#2114 RelaxNG schema parsing: (n=2    25100) 774612 Kb, Δ +16160
memwatch: test_0012_#2114 RelaxNG schema parsing: (n=3    50200) 781596 Kb, Δ +6984
memwatch: test_0012_#2114 RelaxNG schema parsing: (n=4    75300) 788488 Kb, Δ +6892
memwatch: test_0012_#2114 RelaxNG schema parsing: (n=5   100400) 795376 Kb, Δ +6888
memwatch: test_0012_#2114 RelaxNG schema parsing: (n=6   125500) 801984 Kb, Δ +6608
memwatch: test_0012_#2114 RelaxNG schema parsing: (n=7   150600) 809136 Kb, Δ +7152
memwatch: test_0012_#2114 RelaxNG schema parsing: (n=8   175700) 816040 Kb, Δ +6904
memwatch: test_0012_#2114 RelaxNG schema parsing: (n=9   200800) 823032 Kb, Δ +6992
memwatch: test_0012_#2114 RelaxNG schema parsing: (n=10  225900) 830020 Kb, Δ +6988
memwatch: test_0012_#2114 RelaxNG schema parsing: (n=11  251000) 836796 Kb, Δ +6776
memwatch: test_0012_#2114 RelaxNG schema parsing: elapsed 14.401249s
memwatch: test_0012_#2114 RelaxNG schema parsing: slope = 299.12371 (r^2 = 0.990)
F

Finished in 16.431904s, 0.0609 runs/s, 0.1217 assertions/s.

  1) Failure:
MEMORY_SUITE#test_0012_#2114 RelaxNG schema parsing [/home/flavorjones/code/oss/nokogiri/test/helper.rb:351]:
best-fit slope 299.1237145961608 (r^2=0.9902356489234975) should be close to zero
```

Also:
- Run the memory usage tests when the `test:memcheck` rake task is
invoked
- Add a new `test:memory_suite` rake task
- Add a new CI job to run the `test:memory_suite` task

Small changes:
- Change the env var NOKOGIRI_GC to NOKOGIRI_MEMORY_SUITE
- Delete a low-value GC test from 2008


**Have you included adequate test coverage?**

This PR is entirely additional test coverage.


**Does this change affect the behavior of either the C or the Java
implementations?**

N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
topic/ci topic/memory Segfaults, memory leaks, valgrind testing, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

ci: resurrect the memory test suite sparklemotion/nokogiri
1 participant
@flavorjones