0005436: Clarify (or fix) the semantics of Oo.id

(0006411)
garrigue (manager)
2011-12-20 13:58

For the second point, this is oops! : indeed ids should be fresh upon unmarshaling.
It seems that I forgot to implement it (there is a subtle interaction between C code
and ocaml code).

For the first point, we have to check the code.
Maybe it would be better to move the id handling to the C side, since it would avoid the risk of races too.

(0006482)
garrigue (manager)
2011-12-22 08:51

For point 1, ids are guaranteed to be different in different threads:
CamlinternalOO.set_id only reads and assigns to records fields, so no thread switch can occur during its execution.
(At least I believe so. Reading the code in interp.c and systhreads doesn't help me much about
when thread switches can occur.)

For point 2, indeed the behavior was incorrect, and ids were not updated when unmarshaling.
This is now fixed in revision 11930.
One must be careful however that ids cached inside data structures lose their meaning as a result.

(0006503)
frisch (developer)
2011-12-22 12:47

I've added some words of warning the documentation for Oo.id.

(0006546)
xleroy (administrator)
2011-12-28 11:34

I agree with Jacques that we should probably move the generation of fresh IDs to C code, to simplify unmarshaling and to provide clear atomicity guarantees.

(0006549)
dbuenzli (reporter)
2011-12-28 12:55

Would it be possible to also expose the unique id generation as a function in pervasives or Sys ?

That's a useful function beyond ids for objects. I use the id of empty objects as thread safe runtime unique ids in more than one program to implement the keys of type safe heterogenous dictionaries (on Alan's F. suggestion btw. [1]).

let ruid () = Oo.id (object end)

This would avoid linking against Oo in these programs.

Best,

Daniel

[1] http://alan.petitepomme.net/cwn/2010.02.09.html#1 [^]

(0006551)
garrigue (manager)
2011-12-29 07:52

To xleroy:
My fix in revision 11930 keeps the id generation in CamlinternalOO, just exporting the current id through Callback.register_value.
The code is:

let set_id o id =
  let id0 = !id in
  Array.unsafe_set (Obj.magic o : int array) 1 id0;
  id := id0 + 1

Is there any risk of race?
I thought that thread switch could only occur at function calls.

Before doing that, I tried a version with id generation on the C side, but I was afraid that this would make object creation slower. If you think this wouldn't have any impact, I'll revert to that solution.
You can find the patch in experimental/garrigue/caml_set_oid.diffs.

By the way, if the code generating ids in CamlinternalOO is already safe, there is clearly no need to include CamlinternalOO to do that: you just have to duplicate the above 4 lines.

(0006552)
dbuenzli (reporter)
2011-12-29 12:07

> By the way, if the code generating ids in CamlinternalOO is already safe, there is clearly no need to
> include CamlinternalOO to do that: you just have to duplicate the above 4 lines.

Oh right. You mean that what I thought was not thread-safe :

let create =
     let c = ref min_int in
     fun () ->
       let id = !c in
       incr c; if id > !c then assert false (* too many ids *) else id

is in fact thread-safe because no allocation occurs between let id = !c, incr c and the conditional ?

(0007150)
dbuenzli (reporter)
2012-03-24 21:47

Still this would rely on an implementation detail, I'd prefer if something was given to me by the runtime system.

Issue History
Date Modified	Username	Field	Change
2011-12-20 13:15	frisch	New Issue
2011-12-20 13:55	garrigue	Assigned To	=> garrigue
2011-12-20 13:55	garrigue	Status	new => assigned
2011-12-20 13:58	garrigue	Note Added: 0006411
2011-12-20 13:58	garrigue	Status	assigned => acknowledged
2011-12-22 08:51	garrigue	Note Added: 0006482
2011-12-22 08:51	garrigue	Status	acknowledged => resolved
2011-12-22 08:51	garrigue	Fixed in Version	=> 3.13.0+dev
2011-12-22 08:51	garrigue	Resolution	open => fixed
2011-12-22 12:47	frisch	Note Added: 0006503
2011-12-28 11:34	xleroy	Note Added: 0006546
2011-12-28 12:55	dbuenzli	Note Added: 0006549
2011-12-29 07:52	garrigue	Note Added: 0006551
2011-12-29 12:07	dbuenzli	Note Added: 0006552
2012-03-24 21:47	dbuenzli	Note Added: 0007150

Notes
(0006411) garrigue (manager) 2011-12-20 13:58	For the second point, this is oops! : indeed ids should be fresh upon unmarshaling. It seems that I forgot to implement it (there is a subtle interaction between C code and ocaml code). For the first point, we have to check the code. Maybe it would be better to move the id handling to the C side, since it would avoid the risk of races too.

(0006482) garrigue (manager) 2011-12-22 08:51	For point 1, ids are guaranteed to be different in different threads: CamlinternalOO.set_id only reads and assigns to records fields, so no thread switch can occur during its execution. (At least I believe so. Reading the code in interp.c and systhreads doesn't help me much about when thread switches can occur.) For point 2, indeed the behavior was incorrect, and ids were not updated when unmarshaling. This is now fixed in revision 11930. One must be careful however that ids cached inside data structures lose their meaning as a result.

(0006503) frisch (developer) 2011-12-22 12:47	I've added some words of warning the documentation for Oo.id.

(0006546) xleroy (administrator) 2011-12-28 11:34	I agree with Jacques that we should probably move the generation of fresh IDs to C code, to simplify unmarshaling and to provide clear atomicity guarantees.

(0006549) dbuenzli (reporter) 2011-12-28 12:55	Would it be possible to also expose the unique id generation as a function in pervasives or Sys ? That's a useful function beyond ids for objects. I use the id of empty objects as thread safe runtime unique ids in more than one program to implement the keys of type safe heterogenous dictionaries (on Alan's F. suggestion btw. [1]). let ruid () = Oo.id (object end) This would avoid linking against Oo in these programs. Best, Daniel [1] http://alan.petitepomme.net/cwn/2010.02.09.html#1 [^]

(0006551) garrigue (manager) 2011-12-29 07:52	To xleroy: My fix in revision 11930 keeps the id generation in CamlinternalOO, just exporting the current id through Callback.register_value. The code is: let set_id o id = let id0 = !id in Array.unsafe_set (Obj.magic o : int array) 1 id0; id := id0 + 1 Is there any risk of race? I thought that thread switch could only occur at function calls. Before doing that, I tried a version with id generation on the C side, but I was afraid that this would make object creation slower. If you think this wouldn't have any impact, I'll revert to that solution. You can find the patch in experimental/garrigue/caml_set_oid.diffs. By the way, if the code generating ids in CamlinternalOO is already safe, there is clearly no need to include CamlinternalOO to do that: you just have to duplicate the above 4 lines.

(0006552) dbuenzli (reporter) 2011-12-29 12:07	> By the way, if the code generating ids in CamlinternalOO is already safe, there is clearly no need to > include CamlinternalOO to do that: you just have to duplicate the above 4 lines. Oh right. You mean that what I thought was not thread-safe : let create = let c = ref min_int in fun () -> let id = !c in incr c; if id > !c then assert false (* too many ids *) else id is in fact thread-safe because no allocation occurs between let id = !c, incr c and the conditional ?

(0007150) dbuenzli (reporter) 2012-03-24 21:47	Still this would rely on an implementation detail, I'd prefer if something was given to me by the runtime system.

Relationships