6 ms·
3.7X speedup from removing a call to sleep
- khet 14y agoReduced the number of lines in the code base AND improved the product? nice.
- davvid 14y agoDouble nice. A co-worker and I once had a joke where we wanted our code counts for the year to be negative. No one tracks code counts, of course, but it was a fun idea. He eventually replaced a particularly ugly and complicated system with a much simpler system, and thus he met his goal of going into the negative. Less code is better code.
- subhro 14y agoAwesome!! I wonder when this is going to get pushed to mainstream Safari.
- jarek-foksa 14y agoThe patch was committed on 2012-05-17, so it should be already present in Safari for OSX 10.8.
- deleted 14y ago[deleted]
- bvdbijl 14y agoWell, this was four months ago so it might already be in Safari
- ck2 14y agoBut Safari for Windows will be forever stuck without the fix?
- pmjordan 14y agoIt's interesting how spinlocks are making a comeback in user space code. They are widely used in kernel code, but their use has previously been discouraged in application code. As this example illustrates, it's pretty easy to get them wrong...
- subhro 14y agoIndeed spinlocks should be used carefully. But there are some places where you can't avoid them.
- cperciva 14y agoSpin locks are discouraged in FreeBSD kernel code too, except for very low-level areas (serial console, pmap, and interrupt handling come to mind) where you can't safely ask the scheduler to do anything for you. That said, our sleep locks are actually adaptive locks, so on SMP systems they'll spin for a bit before giving up and asking to be descheduled.
- planckscnst 14y agoThey are encouraged in the Linux kernel in places where it is nessesary to lock for a very short time. Using mutexes takes a bit of overhead, so they can't give you very short sleep times; that is when spinlocks have to be used. http://kernel.org/doc/Documentation/spinlocks.txt http://kernel.org/doc/Documentation/spinlocks.txt
- cperciva 14y agoIn FreeBSD the vast majority of sleep lock acquisitions are uncontended and end up being a single compare-and-exchange -- very fast. Spin locks are slightly more work, since we disable interrupts during them.
- raverbashing 14y agoExactly Spinlocks in linux are the "last resource locks", usually in kernel code only in situations where you can't have the scheduler kicking in but can have a race condition (in SMP systems usually) So it's basically interrupt handlers.
- grimebox 14y agoAnyone wanna supply some context? I have no idea what is going on here
- zizee 14y agoSo if I understand this correctly the 3.7x speedup is a speedup in garbage collection and will have a positive effect on the speed of the browser which is nice. But what sort of effect will this have on page rendering? Anyone in the know care to comment on the specifics? edit: improvements to my shoddy wording.
- osivertsson 14y agoMy guess from spending only a few days with the WebKit source is that it will not effect page rendering at all. Possibly not desktop performance at all since I believe GC is done in a separate thread and (from my understanding) you never wait on it, unless you are really memory constrained. Could make a difference on IOS where I believe you actually don't have this GC memory scavenger thread. Anyway, the source is quite readable and everyone working with WebKit on one level or another could benefit from taking a look. At the very least it gives you a feeling for the tremendous effort from a huge number of people that the WebKit project really is.
- zizee 14y agoCool, thanks for the insight.
- azakai 14y agoThe speedup sounds like it is very specific to GC, apparently the WebKit GC relies on this component heavily. I would guess this is relevant only for GC in Safari, since it uses the WebKit JS engine, and not Chrome (which uses a completely different JS engine, V8). It's also likely only a speedup in a specific GC benchmark. If it had helped in say the SunSpider benchmark, I'm pretty sure it would have been fixed a long long time ago.
- othermaciej 14y agoThis code is actually not that specific to GC - this particular spinlock is used by our custom malloc implementation. It's just that our GC creates a different concurrency pattern than most other workloads for the allocator.
- osivertsson 14y agoIt was "only" on one particular benchmark, still it shows that profiling often give you surprising results that nobody familiar with the code anticipated.
- edw519 14y agoTrue story... Client gives me a Help Desk Ticket. User claims batch job use to run in 5 minutes but now runs for hours. The logs confirm this. The commit logs show that an offshore programmer forgot to remove a 10 second sleep command from inside an iteration (for debugging I presume) before promoting to production. I removed it and got a 100X improvement in throughput. My client said that now his user loves him; what did I do to fix it so fast? When I told him that I removed the Sleep, he said, "No! No! No! Change it to a 5 second Sleep so I have something to give him the next time he complains!"
- jakubw 14y agoRelevant: http://thedailywtf.com/Articles/The-Speedup-Loop.aspx http://thedailywtf.com/Articles/The-Speedup-Loop.aspx
- figure8 14y ago" that an offshore programmer forgot..." Unnecessarily mentioning a characteristic of an individual is a sign of selfish, biased, and/or malicious thinking. Do you mention "American programmer" whenever recounting their bugs? It appears you have an agenda of promoting negative stereotypes, presumably in your favor. This is lame. In the future, you'll appear more credible if you omit this bigotry from your comments. I am certain there are thousands of programmers, male, female, gay, straight, onshore and offshore who are much better than you (or me.)
- sigzero 14y agoOver react much?!
- thedeuce 14y agosomeone definitely pee'd in his cheerios this morning Where I work most of the off shore programmers are less skilled though; So I can see how people would use them in that context..its just something from their experience. I don't think we should stake the guy for it...
- 14y ago
- duncanwilcox 14y agoThe code is, appropriately, in the WTF directory.
- aristidb 14y agoThere was a long comment before the sleep. It seems likely that the sleep was there for a reason.
- swah 14y agoAre sleeps() always necessary in multithreaded programming?
- Posibyte 14y agoGenerally, unless I'm working on super low level stuff, I'll avoid sleep()'s at all possible. If I find that I need to spinlock (sleep() to avoid race conditions), there's something wrong with my implementation.
- maximilianburke 14y agoUse of sleep() in multithreaded programming is a bad code smell. There are other safer, more accurate, ways of suspending execution of the current thread until a particular event has occurred, though they may take a little more work to integrate into a program than sprinkling sleep's. Spinlock implementations sometimes use sleep() calls in them to yield the current thread to others that may be ready to run but are currently being blocked. It's still a bad code smell though because spinlocks, at least in user space code, are used to avoid the overhead of yielding to other threads. There are also other locking mechanisms, like mutexes/futexes, that will accomplish the same yielding behavior if it's really needed, like if it's anticipated that the code waiting for the lock may be held up for a while.
- othermaciej 14y agoI am the one who originally added this sleep call to WebKit, when we first imported TCMalloc to use as our custom allocator. It was indeed there for a reason, but that reason is not applicable to WebKit's allocation patterns. TCMalloc was designed for a server workload, over time we have adapted it more to the unique needs of a browser engine. This change may help other operations, but probably not as much as the GC benchmark in question.
- invisible 14y agoThe person making this change seems to have not read the original comment either. It sounds like your change made the previous iteration of that code faster.
- othermaciej 14y agoThis reminds me why I hate comments. They tell you what someone once thought the code did. They don't actually tell you what the code does. You should always trust actual code and actual measurements above comments. The original author of this comment (not me, I just imported the code) believed that it improved performance on a given server-side workload. The person who removed the sleep, Geoff Garen (a colleague and a friend), is a good programmer. I am confident that he read the comment before deleting it. However, his actual performance measurement trumps the comment. BTW, here is Geoff's snarky response on reading the posts here that mention the comment above the sleep() call: <http://www.quickmeme.com/meme/3qqgwz/> http://www.quickmeme.com/meme/3qqgwz/>. I agree with him.
- chris_wot 14y agoWhat's with the old comment "// Sleep for a few milliseconds", followed by the sleep function?
- cpeterso 14y agoWhy did they roll their own spinlock implementation in the first place? They could encapsulate the lock primitives provided by, and optimized for, each platform: CRITICAL_SECTIONs on Windows, futexes on Linux, and pthread_spin_locks elsewhere. iOS and OSX have some Mach-specific spinlocks (OSSpinLockLock?), but I don't have any experience using them.
- tlrobinson 14y agoReminds me of "the speedup loop": http://thedailywtf.com/Articles/The-Speedup-Loop.aspx http://thedailywtf.com/Articles/The-Speedup-Loop.aspx
- jrockway 14y agoTo be fair, it's a sleep(0), which I assume is just the easiest way to get the process to context switch. I assume this is a speed improvement only when locks become ready sooner than a context switch. The real fix would be to make context switches faster, this is just a data-driven tweak rather than a general solution.