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

Fix assertion failure from #12712 #12742

Merged
merged 2 commits into from Nov 15, 2023
Merged

Conversation

jmid
Copy link
Contributor

@jmid jmid commented Nov 14, 2023

This little PR fixes the systhreads assertion failure from #12712.

By good old printf-debugging:tm: I found out that the failure happens
on a boundary condition when 4/5 minor heap pointers align:

 young_start: 4029677568
 young_limit: 4029677568
 young_trigger: 4029677568
 young_ptr: 4029677568
 young_end: 4029685760

Multiple reruns confirmed that the condition is the same (the
addresses naturally vary) every time the assertion fails.

Here's my current understanding:

  • The minor heap uses a young_ptr bump allocator starting
    from young_end and working towards young_start.
  • young_trigger typically marks the mid-point between the two,
    and hitting it triggers a major gc slice.
  • Other domains may set young_limit to UINTNAT_MAX to notify
    a domain (irrelevant in the present systhread test with only 1 domain)

With the above in mind, AFAICS, the above condition may happen when

  • a previous young_ptr allocation just fit perfectly
  • young_trigger is set to young_start by caml_poll_gc_work

This also explains why running with a reduced minor heap
OCAMLRUNPARAM="s=1024" helps to reproduce the failure.

The PR simply proposes to adjust the assertion to account for the
boundary condition.

With the fix, the systhread test from #12712 runs fine across 20 iterations.
To confirm it further, I've also locally run

  • the ocaml compiler test suite
  • the multicoretests test suite both with and without the debug runtime

@gasche
Copy link
Member

gasche commented Nov 14, 2023

(cc @gadmm if you have the time)

@gadmm
Copy link
Contributor

gadmm commented Nov 14, 2023

Yes I think this is the right thing to do. The intent might have been that if young_ptr == young_limit then all (non-zero) allocations will fail anyway (but not pure polling which is a zero-size allocation). But the edge case where young_limit == young_ptr becomes important for memprof where young_limit gets other intermediate values (correctly, there is nothing to poll in this case, but all non-zero allocations must fail). So I think changing the assertion is the correct move.

Copy link
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

Approved on @gadmm's behalf. Thanks both!

@gasche
Copy link
Member

gasche commented Nov 14, 2023

(I assumed the lack of a Changes entry was intentional, but please do feel free to write one.)

@jmid
Copy link
Contributor Author

jmid commented Nov 14, 2023

Thanks for the prompt review! I've added a Changes entry.

Copy link
Contributor

@kayceesrk kayceesrk left a comment

Choose a reason for hiding this comment

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

The change looks good to me. The reasoning

With the above in mind, AFAICS, the above condition may happen when

  • a previous young_ptr allocation just fit perfectly
  • young_trigger is set to young_start by caml_poll_gc_work

also makes sense to me.

@dra27
Copy link
Member

dra27 commented Nov 21, 2023

@Octachron - @jmid is continuously testing the released compiler, so there is use in this being back-ported to 5.1.1. May I do that?

@dra27 dra27 added this to the 5.1.1 milestone Nov 21, 2023
@Octachron
Copy link
Member

Since this is a small assertion fix, ok for backporting to 5.1.1 .

dra27 pushed a commit to dra27/ocaml that referenced this pull request Dec 6, 2023
Fix assertion failure from ocaml#12712

(cherry picked from commit ed3329f)
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.

None yet

6 participants