3.7X speedup from removing a call to sleep

http://trac.webkit.org/changeset/117478

by nexneo • 14 years ago

150 66 14 years ago

trac.webkit.org

khet• 14 years ago

Reduced the number of lines in the code base AND improved the product? nice.

davvid• 14 years ago

Double 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.

subhro2• 14 years ago

Awesome!! I wonder when this is going to get pushed to mainstream Safari.

jarek-foksa• 14 years ago

The patch was committed on 2012-05-17, so it should be already present in Safari for OSX 10.8.

bvdbijl• 14 years ago

Well, this was four months ago so it might already be in Safari

ck2• 14 years ago

But Safari for Windows will be forever stuck without the fix?

pmjordan• 14 years ago

It'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...

subhro2• 14 years ago

Indeed spinlocks should be used carefully. But there are some places where you can't avoid them.

cperciva2• 14 years ago

Spin 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• 14 years ago

They 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

Flow• 14 years ago

It sounds smart but it occurred to me that that I can only think of two situations where a spinlock needs to be promoted. 1. Too many cores access that tiny portion of the system at the same time or 2) a bug(like using a spinlock where something else should be used and a more old-fashioned bug).

Will 1) really happen often enough to not use simple spin locks?

jws• 14 years ago

As an example of where you might use a user space spinlock, consider:

If you have:

… a multithreaded process with shared resources that need locks

… and for the uncontested case your spinlock is much faster than your next fastest lock

… and you have resources for which the probability of a contested access is very close to zero

… and the locks are only held briefly (relative to compute quantum) (especially if they are held shorter than thread context switch)

You may find that the speed gained by using the spinlock is a win even if once in a while you end up burning your compute quantum because a thread got suspended holding a spinlock.

gregsq• 14 years ago

I agree with this assessment. Where the probability of collision is low, and spins are in that case of short duration anyway, and especially if test and set intrinsics are available, spin locks in userspace can be very valuable. They also, besides saving scheduler cycles, require only a common word access, even while that implicates barriers and cache lines.

You need to be careful though.

grimebox• 14 years ago

Anyone wanna supply some context? I have no idea what is going on here

zizee2• 14 years ago

So 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.

osivertsson2• 14 years ago

My 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.

zizee2• 14 years ago

Cool, thanks for the insight.

azakai• 14 years ago

The 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.

othermaciej3• 14 years ago

This 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.

osivertsson2• 14 years ago

It was "only" on one particular benchmark, still it shows that profiling often give you surprising results that nobody familiar with the code anticipated.

edw519• 14 years ago

True 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• 14 years ago

Relevant: http://thedailywtf.com/Articles/The-Speedup-Loop.aspx

figure82• 14 years 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• 14 years ago

Over react much?!

hackerboos• 14 years ago

I think you've gone and assumed that offshore means Indian or Filipino.

My guess is that he was having a jab at the offshoring mentality.

Joeri• 14 years ago

I've worked with good and bad programmers, both on-shore and off-shore. What seems to determine the quality is the standard you set when hiring. If cheap labor matters more than quality, the result is accordingly (and always ends up costing more in the long run).

I suspect the presumption that off-shore means lower quality comes from past experience working with an off-shore team selected based on their perceived lower cost. I've found that when you set the bar high for off-shore hires, regardless of cost, you end up with high-quality people.

ryanwaggoner• 14 years ago

Hogwash. Haven't you ever read a novel or a news story or any kind of narrative? Details add color (uh-oh, I'm sure that'll get you riled up) and they help the listener understand context and picture the story in their head. How boring would life be if every time you read or heard anything negative about someone, it only referred to them as a human?

You can't solve prejudice and bias by pretending that people's race, gender, nationality, height, weight, shoe size, etc. don't exist. They do, they always will, get over it.

lttlrck• 14 years ago

Would "contractor" or "consultant" or "field tech" been unnecessary? Hardly. Offshore doesn't necessarily mean outsourced, it could just as easily mean a satellite office in Europe.

jrajav• 14 years ago

Personally, I didn't take it that way at all. My team interacts very often with another team based in India. It has nothing to do with outsourcing, that's another location of our global company. So, to me, the term "offshore" in this context brings to mind problems of location and time difference, but certainly not some image of a cheap outsourcing shop. The parent post could have just as well meant it this way; in fact, it seems likely.

tmoertel• 14 years ago

What makes you believe that the author of the parent post mentioned the "offshore" characteristic unnecessarily?

When interpreting the story, isn't it important that readers understand that the programmer who added the call to sleep was far removed from both the help desk and the end user of the software and was also unreachable at the time the call was discovered and thus that the author, who worked at the help desk, had to presume that the call was left in the code mistakenly? It's only later in the story that readers (and the author) learn about the ticket-submitter's unusual preferences for satisfying the end user. And it's only then that another possibility dawns upon readers: that, perhaps, the "offshore programmer" knew more about making the stakeholders look good than initially given credit for.

If the author of the parent post is not allowed to reveal that the programmer was working "offshore," how does the story get told at all?

kabdib• 14 years ago

"Competence is inversely proportional to distance." Or perhaps, /perceived/ competence is.

Offshore could imply a number of things. Let's be nice at first and pretend that this means "across the Bay," or maybe on Alcatraz.

My best guess: The offshore person does not have the development culture present in the "mother ship". Best practices are often stuff that you learn in the hallway, from casual conversation, or watching over someone's shoulder. Attachment to the product's quality is probably lessened, too -- distance does this, even in the days of ten-millisecond-scale pings to the other side of the planet -- and it's easier to "go to lunch" on a problem and worry about it later when a floor-full of engineers aren't actively crowding your cubical about a lame checkin.

Hell, I have enough problems getting two /adjacent buildings/ to talk to each other.

Now, add a time-zone difference that further impedes communication. Add siestas, and mismatching holidays, and language barriers. There are more.

Even before we add in a different country's culture, we have severe issues with the average offshore developer's perception of what's important and how to work effectively.

I'm not going to say "Indian / Pakistani / Kiwi programmers can't code their way out of a paper bag", but when you add the stereotypical stuff to all of this (bad management, poor hiring practices, general slap-it-together attitude), it's /bad/, and not unwarranted to mention, even in passing.

cousin_it• 14 years ago

I want a social norm against comments like yours, which say "you're engaging in crime-think pattern #122" instead of "you're wrong". I can't fathom why people fail to realize that such comments kill online discussions. How is one supposed to argue against an accusation of crime-think?

duncanwilcox• 14 years ago

The code is, appropriately, in the WTF directory.

aristidb• 14 years ago

There was a long comment before the sleep. It seems likely that the sleep was there for a reason.

swah• 14 years ago

Are sleeps() always necessary in multithreaded programming?

Posibyte• 14 years ago

Generally, 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• 14 years ago

Use 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.

othermaciej3• 14 years ago

I 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• 14 years ago

The 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.

othermaciej3• 14 years ago

This 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/>. I agree with him.

chris_wot2• 14 years ago

What's with the old comment "// Sleep for a few milliseconds", followed by the sleep function?

cpeterso• 14 years ago

Why 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• 14 years ago

Reminds me of "the speedup loop": http://thedailywtf.com/Articles/The-Speedup-Loop.aspx

jrockway• 14 years ago

To 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.

3.7X speedup from removing a call to sleep

Related Stories