This looks good. What's the best/easiest way to test it?

For testing it, I suppose that frama-c or jane-street code based on ocaml-plugin should torture that a bit.
For performances, the best thing to demonstrate the interest is probably the native toplevel.

Please, can you explain why that work? And perhaps add comments about that? About the invariant involved?

• In order to simplify the code, Can the darkening be done during caml_register_dyn_global?
• Does that still work with aging (if I understand well that can keep some values in the minor heap a little longer)

Thanks

@bobot Thanks for the question, it made me think a bit.
Originaly, my main reason for being convinced was 'it is doing the same thing as non dynlinked roots'.

Now I start to suspect that this is not even necessary to do those Oldify if the fields of the globals are initialized with caml_modify or caml_initialize:
If the value put inside the global is young, it is registered for major to minor pointers and will be oldified by caml_empty_minor_heap. Otherwise, well, it is already old.
Basically, I think that we can consider that global roots, behave like blocks in the major heap (that is, not young). @planar Am I right that we could completely remove both loops that oldify those globals ?
Note that this part does not darken the values, it only move to the major heap.

I have absolutely no idea of the potential interaction with the aging.

if the fields of the globals are initialized with caml_modify or caml_initialize

But they are not, and for two good reasons:
1- caml_modify and caml_initialize assume that the destination pointer is in the minor or major heaps, and will not work if it is outside the heap (like globals are in ocamlopt);
2- calling caml_modify/initialize for every global would increase significantly the code size and compilation times of the initialization functions of compilation units.

For aging, I had to make a slight change to the scanning of global roots, and a similar change should work for dll roots after this patch.

@xavierleroy effectively, I didn't consider the second point.
Yet for the first point I think this works correctly for the globals since they are scanned at the beginning of the major cycle (contrarily to other out of heap blocks). I may be wrong, but I can't see where this would be any different from a block in the major heap.
This does not matter in this case, but I am concerned about that for this other pull request #178 where the invariant that a global block won't be modified twice doesn't hold.

Tested on frama-c, no bug found.

Thanks for the tests. I couldn't find bugs with my testsuite either.
I think that the invariant we may want to ensure is that the globals are not modified once initialized. Would you want a patch ensuring that through mprotect (in debug-runtime mode) (of course also disabling it during a compaction) ?

It would be useful, but don't forget I also use debug-runtime on Mac OS and Windows...

@chambart what is the state of this merge request?

@bobot I think it does not need any modification anymore.
The sugested improved assertions proved to be a bit too tricky to implement (marshal can modify immutable values), so it's not going to be done.

Since this an optimization, can you provide some estimate of the expected gains (or even just a micro-benchmark)?

edit: original message is an error, I mixed with pr #156

@alainfrisch On a quite agressively inlined frama-c this was ~0.3s on a ~1s analysis
On my stupid micro benchmarks for testing that, this could be made arbitrarily beneficial.
In practice I do not know any other software doing as much dynlinking as frama-c, but I imagine that on ocamlnat, it would be hugely beneficial. (I didn't took the time to fix ocamlnat it to test it)

@chambart Did you notice any improvement when starting Frama-C without performing any analysis? Startup times are increasing (which is a problem when running our unit tests), and I'm wondering if this PR might improve things.

On a quite agressively inlined frama-c this was ~0.3s on a ~1s analysis

That sounds great. (LexiFi's application also dynlinks a large part of its code at startup time, so we might see good improvements as well.)

Just curious: how does the "agressively inlined" factor interact with the current proposal?

On 23/06/2015 14:29, alainfrisch wrote:

That sounds great. (LexiFi's application also dynlinks a large part of
its code at startup time, so we might see good improvements as well.)

Just curious: how does the "agressively inlined" factor interact with
the current proposal?

The size of the frame tables is directly linked to the code size and
agressive inlining usualy increase the code size.

But this proposal is about the list of global values, not frame tables, and there is exactly one global per dynlinked module, no?

(Well I see in #177 that you also propose to support multiple globals per unit.)

oups, sorry @alainfrisch and @yakobowski, I'm mixing this pr with #156.
Sorry I don't have numbers ready for this one.

Ok thanks! My intuition is that the gain for this one would be tiny, but the change is tiny as well. I'm in favour of merging.

That said, in general, I strongly advise to provide numbers when proposing an optimization (preferably with realistic benchmarks or even better, real programs; but micro-benchmarks at least otherwise); and also intuitions about when the optimization would be beneficial. This gives some more incentive to look at the proposal.

I just retested (for benchmarking) frama-c with this pr and it segfaulted. I don't know what causes this right now.

Any updates on the segfault? It looks like #178 would be mergeable, but I'm waiting on this one first.

Any updates on the segfault? It looks like #178 would be mergeable, but I'm waiting on this one first.

Wasn't the segfault related to PR#6919: corrupted final_table?

Wasn't the segfault related to PR#6919: corrupted final_table?

No, says pchambart

@chambart two questions:

1. any news of the segfault?
2. is this a prerequisite for flambda?

Not a prerequisite of flambda, and I know how to fix that, but it is a bit more complex than this patch and I don't really have time for that right now.

I'm postponing this PR to 4.04. Speak up if you disagree.

@chambart What is the status for you of this MR?

@bobot, the code here is wrong, and I would still need some time on it. I'm posponing.

Any news on this one?

I'm going to close this; there hasn't been any activity for a year and (forgive me) there are many more important things for @chambart to be getting on with :)