Skip to content

Fix PR #3612.#82

Closed
hnrgrgr wants to merge 1 commit intotrunkfrom
unknown repository
Closed

Fix PR #3612.#82
hnrgrgr wants to merge 1 commit intotrunkfrom
unknown repository

Conversation

@hnrgrgr
Copy link
Copy Markdown

@hnrgrgr hnrgrgr commented Jul 11, 2014

Force minor collection when the unmarshalled value contains custom block with finalizer and has been allocated in the minor heap

Force minor collection when the unmarshalled value contains custom block with finalizer and has been allocated in the minor heap
@bobot
Copy link
Copy Markdown
Contributor

bobot commented Jul 11, 2014

  • The original bug report contains example of the bug, do you think you can add a regression test to the testsuite?
  • Custom block can normally live in the minor heap, why is it problematic for unmarshaling?

@hnrgrgr
Copy link
Copy Markdown
Author

hnrgrgr commented Jul 11, 2014

  • The original bug report contains example of the bug, do you think you can add a regression test to the testsuite?

I will see if possible. Not easy to automatically track the 'C finalizer' execution.

  • Custom block can normally live in the minor heap, why is it problematic for unmarshaling?

This only apply to custom block without finalizer. Custom block with
finalizer are never allocated in the minor heap, see
'caml_alloc_custom' in 'byterun/custom.c'; otherwise, the finalizer
would not be run when the block is dead at the first minor collection.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you add have a finalizer not null?

@bobot
Copy link
Copy Markdown
Contributor

bobot commented Jul 13, 2014

For the testsuite: perhaps we can add a primitive for computing the total number of memory taken by an ocaml program. It is not an easy thing to do in a portable way but it can be useful for tracking memory usage outside the ocaml heap, like for #6294.

Your fix is simple and not intrusive and I think it should be applied. For a future modification of the marshalling format, I just wonder if you can think of a way for allocating directly intern_dest in the major heap in the problematic case, perhaps with a special boolean in the marshalled data.

@gasche
Copy link
Copy Markdown
Member

gasche commented Aug 28, 2014

This was discussed in a development meeting, and we found that forcing a minor collection at each bigarray unmarshalling had an unreasonsable performance cost for some workflows. Alternative approaches have been discussed, including the one proposed by Pierre in #92 , which supersedes the present PR.

@gasche gasche closed this Aug 28, 2014
stedolan pushed a commit to stedolan/ocaml that referenced this pull request Mar 14, 2017
Moved macro 'Assert' from private zone into public space in misc.h
chambart pushed a commit to chambart/ocaml-1 that referenced this pull request Aug 4, 2021
Make `Reloadgen.reload` tail recursive.
stedolan pushed a commit to stedolan/ocaml that referenced this pull request Oct 5, 2021
Make `Reloadgen.reload` tail recursive.
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.

3 participants