Put file descriptors in an Arc #77

Manishearth · 2016-05-23T09:04:02Z

We used to do this manually in servo, but @izgzhen turned it off when consolidating the resource senders

There really isn't much of a reason to not do this; and mpsc::Sender does this already (for a different reason)

asajeffrey · 2016-05-23T13:07:52Z

IRC chat: http://logs.glob.uno/?c=mozilla%23servo&s=23+May+2016&e=23+May+2016#c436462

Summary: I'm not sure how much this helps in the multi-process case, or even in the single-process case where IPCSenders are serialized/deserialized a lot.

Manishearth · 2016-05-23T13:09:31Z

We had Arc<ResourceThread> up until recently which was there to prevent running out of fds. Zhen removed it and we started getting the "too many open files" errors again. It fixes a real error right now that breaks CI.

metajack · 2016-05-23T14:46:19Z

Why did @izgzhen turn it off? Or was it just an unintentional byproduct of his patch?

izgzhen · 2016-05-23T15:02:53Z

@metajack It is mostly like a intentional byproduct. Not all members of ResourceThreads are Arc'd, and the change (from plain-old IpcSender to an Arc'd version) only happens on the half-way between initialisation and actual use, which makes the interface very hard to prose and pass the compiler check.

antrik · 2016-05-24T00:49:18Z

I'm ambivalent about this: on one hand, intuitively it seems wrong to add an extra layer of indirection for everyone, just because of a single user running into problems...

(For the specific issue @izgzhen faced in servo/servo#11189 , it seems to me the solution would be for script-thread simply not to use the "consolidated" ResourceThreads struct; and instead just to keep a separate Arc<CoreResourceThread> variable, as it needs to handle it explicitly anyway when passing it to DocumentLoader. Quite frankly, I'm not very fond of how resource_threads.sender() returns something different depending on an inferred type parameter anyway... It's very opaque IMHO.)

On the other hand, I see that the various Thread structs get cloned in various other places too; and while they might not be as critical, they cause unnecessary resource usage as well. Of course they could all be changed to use Arcs; but then, since it seems a common pattern -- and there is really no good reason ever to clone the actual OsIpcSender structure as far as I'm aware -- it can very well be argued that it's indeed better just to put the Arc into the Sender itself.

One thing to consider though is that for the inprocess platform backend, the OsIpcSender is actually an mpsc::Sender; so wrapping it in another Arc is pointless I guess? Thus probably it would be better to put the Arc in the UnixSender (in the Linux platform backend) instead. (And probably the Mach (MacOS) backend too: while cloning Mach ports should be much cheaper that cloning FDs, I guess it's still unnecessary overhead...)

metajack · 2016-05-24T00:53:36Z

I'd like @pcwalton's input on this.

@Manishearth what do you think about the alternative that @antrik mentioned?

If mpsc::Sender is an Arc under the hood, it seems like making IPCSender an Arc (except not doubling it for in-process) would be fairly consistent with that design. But I am not as familiar with the context here as others on the team.

antrik · 2016-05-24T00:55:18Z

Thinking about it, UnixSender is used in some places "internally" that definitely don't need the Arc... So actually I'm not sure about the best place to put it.

antrik · 2016-05-24T00:57:47Z

ipc.rs

@@ -179,7 +180,7 @@ impl<T> IpcSender<T> where T: Serialize {

    pub fn to_opaque(self) -> OpaqueIpcSender {
        OpaqueIpcSender {
-            os_sender: self.os_sender,
+            os_sender: self.os_sender.clone(),


Why do you need to clone here? Isn't self consumed anyway? Or am I missing something?...

My bad, fixed

Manishearth · 2016-05-26T06:08:56Z

@antrik does this look better to you? I'll look at the osx one later


        Put fd on linux in Arc to avoid too many open files

antrik · 2016-05-26T06:49:48Z

@Manishearth this is kinda funny: most of this change is actually about creating a dedicated FD type, and moving the Clone and Drop implementations there -- which is precisely what I was about to do this very moment, though for other reasons :-)

Indeed my first thought (when I was originally thinking about it a while back) was to put it in an Arc -- only later I concluded that it shouldn't strictly be necessary...

(Note: the title of the PR doesn't really fit any more.)

Manishearth · 2016-05-26T07:04:04Z

heh.

So should I move forward with this and Arcify the mac fds as well?

antrik · 2016-05-26T07:23:01Z

Nitpick: I wonder whether it's appropriate to call it SharedFileDescriptor? It seems to me there is nothing inherently shared about it -- you only make it shared in UnixSender by wrapping an Arc around it... My plan was to go with just FileDescriptor. Might be a matter of what angle you look at it from, though...

(Also, my intention was to use the explicit FileDescriptor type in UnixReceiver etc. as well -- even though this one has no Clone -- for consistency, and because it seems more elegant in general... But this requires some extra consideration; and it''s not really related to your change -- so I guess I can just do a follow-up PR later on.)

Manishearth · 2016-05-26T08:03:42Z

FileDescriptor works

Manishearth · 2016-05-26T08:06:02Z

Actually, we don't need this change in macos, since mach seems to have it's own refcounting api. So this is ready to merge?

pcwalton · 2016-05-26T15:10:02Z

I considered doing this, but I decided against it because of fears of too many allocations. I'm open to it if it actually fixes stuff, though. Do we know for sure that this fixes problems?

Manishearth · 2016-05-26T15:38:36Z

We were already doing this for the resource channel in Servo; I'd like to do it uniformly everywhere. I think the small allocation of an int is shadowed by the IPC buffer and open file costs.

Manishearth · 2016-05-26T16:59:32Z

Right now on my servo checkout some sites do hit the file descriptor limit (which is ~1000 currently for me) if you navigate for a bit

antrik · 2016-05-27T05:05:00Z

@pcwalton well, it's rather clear that cloning of FDs needs to be avoided -- both because of the somewhat relevant overhead, and because of the fixed number of FDs per process on many systems causing serious problems.

What's the best way to achieve that? It's a bit of a tradeoff: doing it at here will add some (very little) unnecessary overhead in cases that definitely won't clone the Sender. The alternative however is to add the Arcs explicitly for all users that might ever clone them, which is way more complicated and error-prone; and on platforms that don't actually need the wrappers, the explicitly added ones would be redundant, thus just shifting the overhead to other use cases... So the defensive programmer is clearly in favour of doing it here; while even the perfectionist and optimisation freak should be swayed by the last point.

Manishearth · 2016-05-27T05:44:23Z

We used to have the perfectionist approach, but we only did it in a couple of places and the too many fds error still popped up.

And again, I feel like we're worrying about a really tiny allocation in face of the cost of an IPC buffer.

antrik · 2016-05-27T09:55:18Z

To my great surprise, this does actually add a (barely) measurable overhead to channel creation (+destruction): adding about 1% to the 4.5 µs these operations take on my system. That's not much in the grand scheme of things (it's some 100 processor clock cycles); yet this is in the same ballpark as a gratuitous FD clone + drop... I for one didn't expect that.

(This is only creation, though -- I haven't tested how long actual cloning takes...)

Note that this is an oldish system (from 2008 or so) running 32 bit Linux -- I wonder whether atomic operations are more efficient on newer systems? (Unfortunately, I don't have access to a more modern system for comparison anymore :-( )

Manishearth · 2016-05-27T09:57:38Z

Hm, interesting. Could you share your benchmarking code so I can try running it?

Note that the primary goal here is to avoid running out of fds, not avoid the overhead of an fd clone. So this being as expensive as an fd clone isn't a big deal.

antrik · 2016-05-27T10:11:58Z

@Manishearth yeah, was just doing that right now: #78 :-)

I don't think this result significantly changes the situation; I mostly mentioned it because it surprised me...

Manishearth · 2016-05-27T10:49:21Z

Mine goes from 11.4 microseconds to 11.7 microseconds. This is a pretty fast machine, not sure why this is happening. But the additional cost isn't much

Manishearth · 2016-05-27T11:53:48Z

So, should we merge this?


        Auto merge of #77 - Manishearth:arc-sender, r=pcwalton

@izgzhen

Put file descriptors in an Arc We used to do this manually in servo, but @izgzhen turned it off when consolidating the resource senders There really isn't much of a reason to not do this; and `mpsc::Sender` does this already (for a different reason) r? @antrik or @pcwalton cc @asajeffrey

Manishearth · 2016-05-31T08:32:04Z

@bors-servo retry force clean

Manishearth · 2016-05-31T08:32:15Z

The build seemed to pass, but it didn't do anything

Manishearth · 2016-05-31T08:32:45Z

@bors r=pcwalton clean

Manishearth · 2016-05-31T08:33:03Z

@bors-servo r=pcwalton retry clean

bors-servo · 2016-05-31T08:33:05Z

💡 This pull request was already approved, no need to approve it again.

bors-servo · 2016-05-31T08:33:06Z

📌 Commit ee08e0b has been approved by pcwalton

bors-servo · 2016-05-31T08:33:07Z

⌛ Testing commit ee08e0b with merge 79cd0c1...


        Auto merge of #77 - Manishearth:arc-sender, r=pcwalton

@izgzhen

Put file descriptors in an Arc We used to do this manually in servo, but @izgzhen turned it off when consolidating the resource senders There really isn't much of a reason to not do this; and `mpsc::Sender` does this already (for a different reason) r? @antrik or @pcwalton cc @asajeffrey

Manishearth · 2016-05-31T12:05:29Z

@bors retry clean

oy vey, Appveyor wasn't fully set up on this

Manishearth · 2016-05-31T12:05:46Z

@bors-servo r=pcwalton

bors-servo · 2016-05-31T12:05:47Z

💡 This pull request was already approved, no need to approve it again.

This pull request is currently being tested. If there's no response from the continuous integration service, you may use retry to trigger a build again.

bors-servo · 2016-05-31T12:05:47Z

📌 Commit ee08e0b has been approved by pcwalton

Manishearth · 2016-05-31T12:06:00Z

@bors-servo retry force clean

Manishearth · 2016-05-31T12:06:33Z

@bors-servo r- clean

Manishearth · 2016-05-31T12:06:41Z

@bors-servo r=pcwalton

bors-servo · 2016-05-31T12:06:42Z

📌 Commit ee08e0b has been approved by pcwalton


        Bump version to 0.2.3

Manishearth · 2016-05-31T12:07:12Z

@bors-servo r=pcwalton

bors-servo · 2016-05-31T12:07:12Z

📌 Commit 7c7a060 has been approved by pcwalton

bors-servo · 2016-05-31T12:07:13Z

⌛ Testing commit 7c7a060 with merge 48137d6...


        Auto merge of #77 - Manishearth:arc-sender, r=pcwalton

@izgzhen

Put file descriptors in an Arc We used to do this manually in servo, but @izgzhen turned it off when consolidating the resource senders There really isn't much of a reason to not do this; and `mpsc::Sender` does this already (for a different reason) r? @antrik or @pcwalton cc @asajeffrey

bors-servo · 2016-05-31T13:22:18Z

☀️ Test successful - status-appveyor, travis

antrik reviewed May 24, 2016
View changes

Manishearth force-pushed the Manishearth:arc-sender branch 2 times, most recently from d903268 to f13eebb May 24, 2016

Put fd on linux in Arc to avoid too many open files

Verified

This commit was signed with a verified signature.

Manishearth Manish Goregaokar

GPG key ID: 3BBF4D3E2EF79F98 Learn about signing commits

Loading status checks…

a9cf20a

Manishearth force-pushed the Manishearth:arc-sender branch from f13eebb to a9cf20a May 26, 2016

Manishearth changed the title ~~Put IPCSender in Arc to preserve file descriptors~~ Put file descriptors in an Arc May 26, 2016

Manishearth mentioned this pull request May 26, 2016

Add file backend support for Blob and related servo/servo#11221

Merged

6 of 6 tasks complete

Manishearth closed this May 31, 2016

Manishearth reopened this May 31, 2016

Manishearth closed this May 31, 2016

Manishearth reopened this May 31, 2016

Bump version to 0.2.3

Verified

This commit was signed with a verified signature.

Manishearth Manish Goregaokar

GPG key ID: 3BBF4D3E2EF79F98 Learn about signing commits

Loading status checks…

7c7a060

Manishearth force-pushed the Manishearth:arc-sender branch from ee08e0b to 7c7a060 May 31, 2016

bors-servo merged commit 7c7a060 into servo:master May 31, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

antrik mentioned this pull request Jun 29, 2016

Store an Arc internally #72

Closed

servo / ipc-channel

Join GitHub today

Put file descriptors in an Arc #77

Put file descriptors in an Arc #77

Conversation

Manishearth commented May 23, 2016

asajeffrey commented May 23, 2016

Manishearth commented May 23, 2016

metajack commented May 23, 2016

izgzhen commented May 23, 2016

antrik commented May 24, 2016

metajack commented May 24, 2016

antrik commented May 24, 2016

This comment has been minimized.

antrik May 24, 2016

This comment has been minimized.

Manishearth May 24, 2016

Manishearth commented May 26, 2016

antrik commented May 26, 2016

Manishearth commented May 26, 2016

antrik commented May 26, 2016

Manishearth commented May 26, 2016

Manishearth commented May 26, 2016

pcwalton commented May 26, 2016

Manishearth commented May 26, 2016

Manishearth commented May 26, 2016

antrik commented May 27, 2016

Manishearth commented May 27, 2016

antrik commented May 27, 2016

Manishearth commented May 27, 2016

antrik commented May 27, 2016

Manishearth commented May 27, 2016

Manishearth commented May 27, 2016

Manishearth commented May 31, 2016

Manishearth commented May 31, 2016

Manishearth commented May 31, 2016

Manishearth commented May 31, 2016

bors-servo commented May 31, 2016

bors-servo commented May 31, 2016

bors-servo commented May 31, 2016

Manishearth commented May 31, 2016

Manishearth commented May 31, 2016

bors-servo commented May 31, 2016

bors-servo commented May 31, 2016

Manishearth commented May 31, 2016

Manishearth commented May 31, 2016

Manishearth commented May 31, 2016

bors-servo commented May 31, 2016

Manishearth commented May 31, 2016

bors-servo commented May 31, 2016

bors-servo commented May 31, 2016

bors-servo commented May 31, 2016