Page MenuHomePhabricator

Fix the Futures iterator so that it always tasks up to the limit
AbandonedPublic

Authored by hach-que on May 14 2014, 7:37 AM.
Tags
None
Referenced Files
F14021235: D9115.diff
Wed, Nov 6, 6:45 AM
Unknown Object (File)
Sun, Oct 13, 3:35 PM
Unknown Object (File)
Sun, Oct 13, 3:35 PM
Unknown Object (File)
Sun, Oct 13, 3:04 PM
Unknown Object (File)
Sun, Oct 13, 8:00 AM
Unknown Object (File)
Oct 2 2024, 3:32 AM
Unknown Object (File)
Sep 20 2024, 4:23 PM
Unknown Object (File)
Sep 19 2024, 7:01 AM

Details

Summary

Fixes T5042.

The Futures iterator currently only takes the first 8 items from the waiting list and uses that as the working set. Unfortunately items are only popped from the top of the waiting list as fast as they are processed in PHP, which means if the tasks are executing faster than PHP is processing the results, then the number of running tasks will be lower than the limit.

This fixes the issue by counting up to the limit when taking items from the waiting list, not counting items that are resolved (but still adding them to the working set so we can respond to them exiting). Thus it ensures that there are always limit unresolved futures active, with the a certain number of the working set already resolved and ready to be processed.

Test Plan

Ran a linter with a limit of 8 concurrent futures.

Diff Detail

Repository
rPHU libphutil
Branch
futures-fix
Lint
Lint Skipped
Unit
Tests Passed
Build Status
Buildable 455
Build 455: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

hach-que retitled this revision from to Fix the Futures iterator so that it always tasks up to the limit.
hach-que updated this object.
hach-que edited the test plan for this revision. (Show Details)
hach-que added a reviewer: epriestley.

Performance test from running time arc lint --everything --never-apply-patches --output summary with and without the patch:

Without the patch: 13m53s
With the patch: 4m30s

Is this diff still valid? It seems worthwhile fixing this.

I believe it's still valid, yes.

epriestley edited edge metadata.

I was never able to reproduce this (after trying at length) and still don't believe there's an actual bug here. This should be trivial to reproduce if the described behavior is reliably the actual behavior.

I want to see a repro case locally before pursuing this.

This revision now requires changes to proceed.May 13 2015, 2:02 PM

I don't believe we (personally) use any linters or unit test engines that aggressively use lots of processes any more, so I'm just going to abandon this.