1379263 - PoisonIOInterposterMac.cpp should not fstat in IsValidWrite if the write is for an IPC message

Status: RESOLVED INVALID

(Core :: XPCOM, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

jrmuizel and I noticed this on the compositor thread while looking at profiles:

http://searchfox.org/mozilla-central/rev/f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/xpcom/build/PoisonIOInterposerMac.cpp#158-166

This seems to occur when we send IPC messages. Seems a bit odd that we'd fstat for IPC messages unless they happen to be hitting the disk somehow.

Perhaps we could change the order of these so that for IPC messages, we never fstat?

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Hey spohl, is our thinking right on this one? Do you know if we're safe to re-order these?

Comment 2

[Stephen A Pohl [:spohl]]

Sorry about the delay. Your reasoning seems to make sense to me, but I can't say for sure that we're safe to reorder these calls.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

(In reply to Stephen A Pohl [:spohl] from comment #2)
> Sorry about the delay. Your reasoning seems to make sense to me, but I can't
> say for sure that we're safe to reorder these calls.

Hm, okay. Maybe aklotz knows?

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

DO IT

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

If I read the code correctly, the fstat is required because IsIPCWrite actually checks the st_mode to determine if the fd is a fifo or unix socket.

fstat might create disk io if the fd is actually a file but we are writing to disk anyway. For a pipe it should not have a big impact.

Closing this as invalid unless we found the fstat indeed is expensive.