Skip to content

Allow finalizers on custom blocks in the minor heap, fix pr 3612#92

Closed
chambart wants to merge 5 commits intoocaml:trunkfrom
chambart:finalizers_in_minor_heap
Closed

Allow finalizers on custom blocks in the minor heap, fix pr 3612#92
chambart wants to merge 5 commits intoocaml:trunkfrom
chambart:finalizers_in_minor_heap

Conversation

@chambart
Copy link
Contributor

@chambart chambart commented Aug 19, 2014

This subsume the pull request #82

Note that this also make functions like Bigarray.Array1.sub a lot faster:

  for i = 0 to n do
    ignore(Array1.sub a 3 5)
  done

Is more than twice faster.

@chambart
Regression test for PR 3612
f54ebf5
@chambart
Allow allocating custom blocks with finalizers in the minor heap.
f95590f 
When allocating this kind of blocks to the minor heap, they are
added to 'caml_finalize_table' which is traversed on
'caml_empty_table' to check if any of such block is dead.
@chambart
Handle custom block with finalizers in deserialization. Fix ocaml#3612
ee05063
@chambart
Factorisation of registration to extensible tables
5a0cff9
@bobot
Copy link
Contributor

bobot commented Aug 27, 2014

For me, perfect pull request:

  • new tests
  • factorization of previous code
  • small and clear

Perhaps just some additional comments.

bobot
bobot reviewed Aug 27, 2014
View reviewed changes
byterun/minor_gc.c
Copy link
Contributor

@bobot bobot Aug 27, 2014

Choose a reason for hiding this comment

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

Could you add that for "the finalizer of the others will be called during mark_sweep when they became unreachable?". I just forget that custom_block are not finalized as usual ocaml values....

Copy link
Contributor Author

@chambart chambart Aug 29, 2014

Choose a reason for hiding this comment

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

I am not sure this is the right place to add that kind of comment: it may become obsolete if sweep_slice evolve.

@gasche gasche mentioned this pull request Aug 28, 2014
Closed
@bobot
Copy link
Contributor

bobot commented Nov 11, 2014

This PR have been discussed during the last developers' meeting, where I had the pleasure to be. To sum-up:

  • 5a0cff9: Should not be in this pull-request. It should go in another where the function would be in a .h and inlinable.
  • Otherwise nice solution to this long standing problem
  • Ok for integration
  • But it would be interesting to test on real world example if it is really a win to always have the bigarray proxy in the minor heap.

@gasche
Copy link
Member

gasche commented Jul 26, 2015

The patch should be merge-ready, but on my machine many backtrace-related tests fail because of it. Can you reproduce the failures?

List of failed tests:
    tests/backtrace/backtrace.ml
    tests/backtrace/backtrace.ml
    tests/backtrace/backtrace.ml
    tests/backtrace/backtrace2.ml
    tests/backtrace/raw_backtrace.ml
    tests/backtrace/backtrace_deprecated.ml
    tests/backtrace/backtrace_slots.ml

@gasche
Copy link
Member

gasche commented Jul 26, 2015

PS: I have pushed the rebased version of this patch (moving headers to the caml/ namespace, plus a Changelog entry, two in fact) to a private branch PR#92-rebased. See comparison with trunk.

@xavierleroy
Copy link
Contributor

xavierleroy commented Aug 2, 2015

I reviewed the rebased version and I'm (more than ever) OK for merging.

@gasche
Copy link
Member

gasche commented Aug 2, 2015

So the backtrace problem seems to be due to a bug in byterun/backtrace.c, I'll fix it before merging this one.

This was referenced Aug 3, 2015
Closed
Closed
@gasche
Copy link
Member

gasche commented Aug 16, 2015

Merged in trunk. Thanks for the good work!

Congratulations for fixing the oldest fixed issue in the release cycle for now. The next contender, PR#4539, is 3 years older.

@gasche gasche closed this Aug 16, 2015
@bobot
Copy link
Contributor

bobot commented Mar 3, 2016

For reference this change had consequences on lablgtk: http://caml.inria.fr/mantis/view.php?id=7162

@DemiMarie
Copy link
Contributor

DemiMarie commented Mar 4, 2016

@bobot I think that the breakage is a bug in lablgtk.

This was referenced Mar 14, 2019
Closed
Closed
Closed
Closed
chambart pushed a commit to chambart/ocaml-1 that referenced this pull request Aug 4, 2021
@xclerc
Better fatal error message in Selectgen. (ocaml#92)
669048d
stedolan pushed a commit to stedolan/ocaml that referenced this pull request Oct 5, 2021
@xclerc @mshinwell
Better fatal error message in Selectgen. (ocaml#92)
39b1e36
sadiqj pushed a commit to sadiqj/ocaml that referenced this pull request Feb 21, 2023
@mshinwell
Bootstrap (ocaml#92)
942f2ab
stedolan pushed a commit to stedolan/ocaml that referenced this pull request Mar 21, 2023
@riaqn
Squashed 'ocaml/' changes from a09392d..c703f5f
bcec5fe 
c703f5f Incorporate upstream comments into type-variable refactor (ocaml#121)
362ba23 Constrain curry modes to increase along applications (ocaml#108)
b1f0cf9 Simplify the extension handling (ocaml#114)
4fd53a1 Remove pat_mode from typedtree (ocaml#105)
cf6fcbc Handle attributes on lambdas with locally abstract types (ocaml#120)
5fa80fe Don't track attributes inside attributes for warning 53 (ocaml#115)
8a69777 Handle unclosed `[: ... :]` patterns (via `Generic_array` machinery) (ocaml#117)
b0737f4 Add promote-one Makefile target (ocaml#118)
c6ad684 Refactoring and fixes around module lookup (ocaml#107)
b0a6495 Add documentation for global constructor arguments (ocaml#69)
dd79aec Print `nlocal` in the `-d(raw)lambda` output (ocaml#112)
8035026 Fix `nlocal` in the generated Lambda for list comprehensions (ocaml#113)
afbcdf0 Immutable arrays (ocaml#47)
bfe1490 fix several issues when removing exp_mode (ocaml#110)
8f46060 Better error message for under-applied functions (ocaml#74)
27331d8 Consistently use Lmutvar or Lvar in comprehensions (ocaml#111)
01e965b Skip failing test for now
0131357 Fix test case to use comprehensions_experimental
22a7368 Temporarily disable list comprehensions tests due to locals bug
e08377d Make `comprehensions` into `comprehensions_experimental` for now (ocaml#109)
947cf89 List and array comprehensions (ocaml#46)
bd9e051 remove exp_mode from typedtree (ocaml#100)
a9268d2 Fix misplaced attribute warning when using external parser (and some cleanup) (ocaml#101)
2b33f24 Refactor toplevel local escape check (ocaml#104)
ed2aec6 Comment functions exported from TyVarEnv.
87838ba Move new variable creation into TyVarEnv.
a3f60ab Encapsulate functions that work with tyvars
43d83a6 Prevent possibility of forgetting to re-widen
2f3dd34 Encapsulate context when narrowing type env't
d78ff6d Make immediate64 things mode cross (ocaml#97)
aa25ab9 Fix version number (ocaml#94)
d01ffa0 Fix .depend file (ocaml#93)
942f2ab Bootstrap (ocaml#92)
05f7e38 Check Menhir version (ocaml#91)
1569b58 Move the CI jobs from 4.12 to 4.14. (ocaml#90)

git-subtree-dir: ocaml
git-subtree-split: c703f5f
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
Remove postcss-cli from official dependencies (ocaml#92)
b46d154
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
@tmattio
Merge pull request ocaml#92 from ocaml/merge-frontend
145d8f6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@chambart @bobot @gasche @xavierleroy @DemiMarie