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
BLocking stubs not releasing the runtime in stdlib and unix #6276
Comments
Comment author: @diml I don't think there is any particular reason not to release the runtime in these functions? I'm preparing a patch. |
Comment author: @diml Patch attached. If there is no objections I'll commit it. |
Comment author: @gasche Have I correctly understood that then change involves doing copy for the string arguments (usually file paths) of those functions, when there often was not before? Would that make a performance difference for, say, recursive traversal of a large filesystem? |
Comment author: @diml
Yes
It is going to be slightly slower, yes. With the attached traverse.ml, on a hierarchy of 1M files, I get a 3% slowdown. I can understand that all these copy are a bit frustrating, but not releasing the lock means that you can block all threads because one of them is accessing a NFS. |
Comment author: @gasche
That seems reasonable.
I agree copying and releasing is a better compromise. I'm not familiar with the FFI, so I can't say whether your patch is correct. (There are good explanations of enter_section/leave_section in http://d.hatena.ne.jp/camlspotter/20100309/1268111257 ). Can you explain a bit when exactly do you need to add CAMLparam/CAMLreturn? My understanding was that they were need when the GC is called (and caml_stat_alloc{_string} does not call the GC). Does releasing the lock require using those? Why does your patch add them to caml_sys_remove, but not caml_sys_rename, for example? |
Comment author: @diml
Some of the stubs are using the [path] variable for the error message after the call. So we need to ensure it is still valid.
Yes, the GC might run in another thread for instance.
caml_sys_remove uses [path] for the error message but caml_sys_rename doesn't. I guess it wouldn't harm to add them everywhere. |
Comment author: @diml commited, revision 14384 and 14385. |
Comment author: @yallop I think 'File_offset_val' needs to be moved outside the blocking section in 'unix_truncate_64', and the call to 'access' in 'unix_access' needs to use 'p' instead of 'String_val(path)'. |
Comment author: @diml Indeed. Fixed in commits 14387 and 14388. Thanks! |
Comment author: @yallop Another thought: it's probably very slightly better to have the calls to caml_stat_free inside the blocking sections. |
Comment author: dsheets I believe ftruncate, fchown, and fchmod also need to have the lock released. |
Comment author: @diml Indeed. Fixed in commits 14425 and 14426. Thanks! |
git-svn-id: http://caml.inria.fr/svn/ocaml/version/4.01@14385 f963ae5c-01c2-4b8c-9fe0-0dff7051ff02
Original bug ID: 6276
Reporter: @diml
Assigned to: @diml
Status: closed (set by @xavierleroy on 2015-12-11T18:25:42Z)
Resolution: fixed
Priority: normal
Severity: minor
Target version: 4.01.1+dev
Fixed in version: 4.01.1+dev
Category: runtime system and C interface
Has duplicate: #4995
Related to: #4801
Monitored by: @gasche @hcarty @dbuenzli
Bug description
The following functions might block and do not release the runtime lock:
Sys.chdir
Sys.close
Sys.file_exists
Sys.is_directory
Sys.readdir
Sys.remove
Sys.rename
Unix.readdir
Unix.opendir
Unix.close
Unix.closedir
Unix.rename
Unix.remove
This can be a problem when using NFS for instance (that's how we noticed it).
File attachments
The text was updated successfully, but these errors were encountered: