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 a bug in [caml_modify_generational_global_root] #8656

Merged
merged 1 commit into from May 6, 2019

Conversation

@jhjourdan
Copy link
Contributor

commented May 3, 2019

When the old value was static (i.e., boxed, but outside of the major and the minor heaps) but the new value pointed to one of the two heaps, then the root was not re-inserted in the lists.

@jhjourdan jhjourdan force-pushed the jhjourdan:gen_root_bug branch from b7d3693 to 812313f May 3, 2019
@jhjourdan

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

This is a safety-critical bug for anyone using this API. This should be back-ported to any OCaml version which is still maintained.

@jhjourdan

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

I pushed a new version of the patch. I makes explicit in a comment the invariant of generational roots, and fixes the dual bug, where a root becomes a pointer outside of the heap, and the root was not removed.

@dra27

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

The test passes in bytecode - the bug should be triggerable in bytecode too?

@gasche

This comment has been minimized.

Copy link
Member

commented May 4, 2019

If I understand correctly, this is a sort of follow-up on your earlier fix to the removal logic for generational roots, #607. (I guess both fixes come from statmemprof hacking.)

@gasche

This comment has been minimized.

Copy link
Member

commented May 4, 2019

I looked at the code. It seems to me that:

  1. The logic is fairly complex (it was simple at the start, and became complex when fixing bug #4704 which added the initial handling of changes between boxed and unboxed, now tuned by this PR to take out-of-heap blocks into account).
  2. The code is not very efficient due to the systematic uses of the Is_in_* macros that access the page table (the new, more correct code makes this worse, by adding a Is_in* check after a lot of Is_block conditionals).

For example, if you replace a young value by a static value, or a static value by a young value, in both cases, if I read the code correctly, there will be 2 heap-classification checks for the young value, and 2 heap-classification checks for the old value.

I think that both problems could be solved at once by adding a classification function (in the spirit of Classify_addr(a), but tailored to generational roots) that takes a value and returns a value representing whether the address should be untracked, tracked in the young list or tracked in the old list

The resulting code would look as follows (written directly in this message box: unparsed, untested, etc), where it is easy to check that all cases are covered, and at most one address-classification computation is made for each value.

enum gc_root_class {
  YOUNG,
  OLD,
  UNTRACKED
};

enum gc_root_class classify_gc_root(value v);

CAMLexport void caml_modify_global_roots(value *r, value new_val) {
  value old_val = *r;
  enum gc_root_class old_class = classify_gc_root(oldval);
  enum gc_root_class new_class = classify_gc_root(newval);
  if (old_class == YOUNG) {
    // if new_class is OLD, we leave the fixup work to the next minor GC
    if (new_class == UNTRACKED)
      caml_delete_global_root(&caml_global_roots_young, r);
  }
  else if (old_class == OLD) {
    if (new_class == YOUNG) {
     caml_delete_global_root(&caml_global_roots_old, r);
     caml_insert_global_root(&caml_global_roots_young, r);
     // Note: even if old_class is OLD, it may still be in the young roots.
     // The code is still correct in that case, as
     // deleting an absent root and inserting a present root are no-ops.
    } else if (new_class == UNTRACKED) {
      // the old value may still be in the young roots
      caml_delete_global_root(&caml_global_roots_young, r);
      caml_delete_global_root(&caml_global_roots_old, r);
    }
  }
  else { // (old_class == UNTRACKED)
    if (new_class == YOUNG)
      caml_insert_global_root(&caml_global_roots_young, r);
    else if (new_class == OLD)
      caml_insert_global_root(&caml_global_roots_old, r);
  }
  *r = new_val;
}
@jhjourdan

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

If I understand correctly, this is a sort of follow-up on your earlier fix to the removal logic for generational roots, #607. (I guess both fixes come from statmemprof hacking.)

Interesting, I completely forgot about that one! Indeed, this is the result of statmemprof hacking. Actually, I would say this is a different bug, since that one involves static data, while #607 only involves data in the heap. Hopefully, after 3 fixes, this function is now correct.

@jhjourdan

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

The test passes in bytecode - the bug should be triggerable in bytecode too?

The way the test works in native mode is that (1,1) is pre-allocated in static region. But the bytecode compiler does not perform this optimization. I will try with atoms.

@jhjourdan

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

I think that both problems could be solved at once by adding a classification function (in the spirit of Classify_addr(a), but tailored to generational roots) that takes a value and returns a value representing whether the address should be untracked, tracked in the young list or tracked in the old list.

Ok, let me try to propose something like this.

@jhjourdan jhjourdan force-pushed the jhjourdan:gen_root_bug branch from 21ecd4f to 9f703db May 4, 2019
@jhjourdan

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

@gasche, here is a new version, which is hopefully easier to read.

Still, the optimization that lets us not do anything in the young-to-old case makes things significantly more complex. Do we really need it? Is this a use case which is particularly frequent?

Copy link
Member

left a comment

Yes, I think that the new implementation improves things (switch in C is clearly the semantically-appropriate construct, but yet never really pleasant to use...). See comments inline.

runtime/globroots.c Show resolved Hide resolved
runtime/globroots.c Show resolved Hide resolved
runtime/globroots.c Outdated Show resolved Hide resolved
runtime/globroots.c Show resolved Hide resolved
When the old value was static (i.e., boxed, but outside of the major
and the minor heaps) but the new value pointed to one of the two
heaps, then the root was not re-inserted in the lists.

Dually, when the new value is static but the old only points to one of
the two heaps, then the root has to be removed, otherwise it will stay
in the lists, and be scanned by the GC even after being unregistered.
@jhjourdan jhjourdan force-pushed the jhjourdan:gen_root_bug branch from 9f703db to 8d4750b May 5, 2019
@gasche

This comment has been minimized.

Copy link
Member

commented May 5, 2019

I like the new implementation, thanks! I'd like to give a full re-read of the code before approving, and probably won't have time before tomorrow.

@gasche
gasche approved these changes May 6, 2019
Copy link
Member

left a comment

I re-read the PR and I am convinced that the new code is correct.

On the question of whether the complexity of tolerating old values in the young list is worth it: I am, as @jhjourdan, unconvinced.

This optimization speeds up modifications of roots in the case where we are replacing a young value with an old value, at the cost of slowing down deletion of old values, which must also be removed from the young list.

I don't have a clear mental model of the life cycle we can expect for generational roots, but I would expect that the use-case is to write (at registration time and a root-modification time) untracked constants and young values that later become old, rather than old values, and that deletion is more common than modification. So this optimization is not a clear win.

I looked for usages of the generational API online, and I found:

  • Some usages to register OCaml-shaped data created from the C side, such as elm_box_wrap.c in ocaml-efl, where the registered values are young. (I haven't found any instance where the roots are un-registered, so there may be a leak in this code.)

  • Some usages to register callbacks for various C APIs, ide_mac_stubs.c in CoqIDE, gnutls.c in ocamlnet: in this case I expect the closure to be constructed from the OCaml side as an argument or defined close by, so either static (if closed) or young (if just allocated)

  • Generic usages in C++ constructor/destructor pairs in generic interoperability libraries like this usage in mlsmoke. In this case it's hard to predict the class of the registered value, but there are only deletions, no modifications.

That said, it seems that making this optimization is outside the scope of the present PR, so I would suggest that we deal with the bugfix and code clarification now, and that you write a second PR to remove the optimization -- if you care -- on top of this one, or after this one is merged.

@jhjourdan

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

That said, it seems that making this optimization is outside the scope of the present PR, so I would suggest that we deal with the bugfix and code clarification now, and that you write a second PR to remove the optimization -- if you care -- on top of this one, or after this one is merged.

LGTM. Not sure I do care, though.

@gasche

This comment has been minimized.

Copy link
Member

commented May 6, 2019

I just started a precheck test (I was mildly annoyed by the unrelated Appveyor failure: there is a small chance that the new code fails with some C compiler somewhere), and will merge if it passes.

https://ci.inria.fr/ocaml/job/precheck/259/

@gasche gasche merged commit 7f9913b into ocaml:trunk May 6, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jhjourdan

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

Thanks!

Don't you think this should be backported to e.g., the 4.09 branch?

gasche added a commit that referenced this pull request May 7, 2019
Fix a bug in [caml_modify_generational_global_root]

(cherry picked from commit 7f9913b)
@gasche

This comment has been minimized.

Copy link
Member

commented May 7, 2019

Yes for 4.09 (I cherry-picked in 2e9e326), no for 4.08.

@jhjourdan

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

@gasche : Could you add yourself as a reviewer in the Changes file? Currently, it shows ???.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.