Skip to content

Conversation

@uri-canva
Copy link
Contributor

@uri-canva uri-canva commented Aug 4, 2022

Using dyld interposing as documented in:
https://opensource.apple.com/source/dyld/dyld-97.1/include/mach-o/dyld-interposing.h.auto.html
Following the example here:
https://github.com/NixOS/nixpkgs/blob/9d452bdebedb1c47adaabd40e61a742f9f07b54c/pkgs/build-support/libredirect/libredirect.c

I've unconditionally passed both DYLD_INSERT_LIBRARIES and LD_PRELOAD, reasoning that whichever runtime linker we're using will ignore the environment variable it doesn't use, and we'll unset both in the initializer anyway.

@uri-canva uri-canva mentioned this pull request Aug 5, 2022
@uri-canva
Copy link
Contributor Author

@lilyball sorry for the unsolicited ping, but I've seen you participate in some of the issues around faster direnv / nix flakes evaluation caching on darwin. Would you be interested in helping review this PR?

@uri-canva
Copy link
Contributor Author

ping @xzfc

1 similar comment
@uri-canva
Copy link
Contributor Author

ping @xzfc

@lilyball
Copy link

I don't use cached-nix-shell (I hadn't heard of it until now, and my normal driver is nix-direnv with flakes which already caches nicely), but I took a quick skim through this. I haven't used DYLD_INSERT_LIBRARIES before, or dyld interposing, but I'm aware of both and the idea looks okay.

However, I don't understand why you're using a dispatch_semaphore_t as a mutex. A dispatch semaphore is not as good as an actual mutex, because it can't do priority donation. macOS's pthread mutex is perfectly fine, and I believe it even defaults to being unfair these days in case this matters (you can set this with pthread_mtuexattr_setpolicy_np(&attr, PTHREAD_MUTEX_POLICY_FIRSTFIT_NP) if you so desire, and <pthread/pthread_spis.h> has the handy-looking PTHREAD_FIRSTFIT_MUTEX_INITIALIZER, but I don't believe this is necessary). macOS also has os_unfair_lock() which is like a spinlock but does priority donation, but since this code was already using a pthread mutex I assume you don't want to replace it with a spinlock-type lock.

The libdispatch equivalent for a mutex is actually a serial dispatch_queue_t, with blocks enqueued using dispatch_sync() (if you can use Apple's C blocks) or dispatch_sync_f(), but that's a bigger change and has no benefit over just using pthread_mutex anyway in the purely synchronous case.

I do think splitting the one mutex into two is a good idea so you can avoid recursion, although the alternative would have been to split print_log() into a separate print_log_locked() that you can call if you already have the mutex. macOS does define PTHREAD_RECURSIVE_MUTEX_INITIALIZER (see the #if conditions though), but avoiding recursive mutex shoudl be a performance win anyway.

@xzfc xzfc merged commit cb1c60a into xzfc:master May 24, 2023
@xzfc
Copy link
Owner

xzfc commented May 24, 2023

Thanks.

@NorthIsUp
Copy link

new release please?

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.

4 participants