Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Android Project Changeset 4f8b683: libc/memset.c

review.source.android.com

Post reply on HN
Print view View original

41–50 of 93 posts

Re: Android Project Changeset 4f8b683: libc/memset.c

#41

Post by s3graham »

queensnake wrote: post #29
klodolph wrote: post #22

Earlier quoted context omitted.

"Pretty severe" lack of professionalism? It's not unprofessional not to turn the warnings all the way up and sift through each and every one. Some of us work code inherited from other people with different coding guidelines, and some of the warnings will generate loads of messages for code that is not only technically correct but stylistically correct as well. The unused parameter warning is one of the biggest offend…

| The unused parameter warning is one of the biggest offenders in this area. And if you have a hundred warnings for unused parameters, which ones do you pay attention to? Well, so then you're exposed to this error. Do: void foo(int x) { (void)x; } to silence unused parameter warnings.

(void)x does not work on all compilers to silence the warning. gcc + cl != C.

Re: Android Project Changeset 4f8b683: libc/memset.c

#42

Post by s3graham »

tptacek wrote: post #35
humbledrone wrote: post #32

Earlier quoted context omitted.

I would probably agree that most software shops in the world are not halfway decent. The benefits of tracking down and squelching such warnings would have very likely caused this bug to never exist. The point is that you make "compiles without warnings" a policy, and you build a habit of maintaining that policy -- after a while it begins to take almost no effort at all.

Do you write C code?

(and for more than one platform?)

Re: Android Project Changeset 4f8b683: libc/memset.c

#43

Post by humbledrone »

tptacek wrote: post #37
queensnake wrote: post #29

Earlier quoted context omitted.

| The unused parameter warning is one of the biggest offenders in this area. And if you have a hundred warnings for unused parameters, which ones do you pay attention to? Well, so then you're exposed to this error. Do: void foo(int x) { (void)x; } to silence unused parameter warnings.

Go look at Google code search, which includes vast quantities of code written by professional teams and (often later) open sourced. Very little of it casts expressions to (void). When I started at Arbor, I got rid of all the (void) casts. What an eyesore. Very little C code is const-correct either. Most of the C devs I know make fun of const-correctness.

| What an eyesore.

It's only an eyesore if you don't acknowledge the benefits that strict warnings can bring to the table.

| Most of the C devs I know make fun of const-correctness.

I for one don't mind putting in a tiny bit of upfront effort so that my compiler can double-check my work. Do the C devs you know also make fun of assert?

Re: Android Project Changeset 4f8b683: libc/memset.c

#44

Post by tptacek »

humbledrone wrote: post #43
tptacek wrote: post #37

Earlier quoted context omitted.

Go look at Google code search, which includes vast quantities of code written by professional teams and (often later) open sourced. Very little of it casts expressions to (void). When I started at Arbor, I got rid of all the (void) casts. What an eyesore. Very little C code is const-correct either. Most of the C devs I know make fun of const-correctness.

| What an eyesore. It's only an eyesore if you don't acknowledge the benefits that strict warnings can bring to the table. | Most of the C devs I know make fun of const-correctness. I for one don't mind putting in a tiny bit of upfront effort so that my compiler can double-check my work. Do the C devs you know also make fun of assert?

I don't acknowledge the benefits of casting things to (void). Therefore: definitely an eyesore.

And, no. Nobody makes fun of assert. Which rather makes my point for me.

Re: Android Project Changeset 4f8b683: libc/memset.c

#45

Post by ars »

tptacek wrote: post #38
ars wrote: post #33

Earlier quoted context omitted.

How would you write this function? Fully portable, so no ASM.

You'd unroll the loop. But knowing the average workload for memset(), it probably wouldn't be a perceptible win.

With todays CPU, particularly with branch prediction, and cache, loop unrolling is not always better.

In fact I bet the performance would be worse, especially because you have to worry about setting areas of memory that are not a multiple of the loop unroll size. (I guess you could use duff's device.)

I actually tested it while ignoring that, and the unrolled version took 34.551 seconds vs 34.239 for the regular version (but those number are meaningless since the variation in time between runs is greater than the difference in runtime between versions).

And see: http://lkml.indiana.edu/hypermail/linux/kernel/0008.2/0171.h... - loop unwinding is not worth it anymore.

So I ask you again pkaler: how would you speed this up?

Re: Android Project Changeset 4f8b683: libc/memset.c

#46

Post by wvenable »

jey wrote: post #23
humbledrone wrote: post #18

Earlier quoted context omitted.

Huh? Any halfway decent software shop will have a policy of compiling with all warnings enabled, and fixing all sources of warnings (with only very rare exceptions). It is extremely unprofessional to disable or ignore warnings such as the "unused function parameter" warning. If you disagree, I respectfully submit that I would never let you near a C project that I was working on.

The unused function parameter warning is usually one of the most annoying and least useful. I routinely compile with "-Wall -Wno-unused" because I don't want to litter my code with casts to void.

What's with all the unused function parameters? Shouldn't that be an edge case?

Re: Android Project Changeset 4f8b683: libc/memset.c

#47

Post by faragon »

spolsky wrote: post #19

Aren't there are processor-specific assembler versions of memset for common chips? There are usually assembler versions (for example using REP MOVS on Intel which is light years faster) or even unrolled C versions (typically setting 8 or 16 bytes per iteration) that are used. I suspect this might be "fallback code" that doesn't actually get executed on any chips that anybody really uses.

For this case, the required x86 opcode could be "rep stosX" (just write), and not "rep movsX" (copy). Anyway, on x86 processors, "rep movsX" was faster from 8086 to 80386, but since the 80486, it is faster to do assembly unrolling for reducing the jump penalty (as the "rep movsX" doesn't do unrolling, being its conditional jump a penalty). On CPUs with prefetch hardware support -e.g. SSE2-, you could still speed it up a bit more, by fetching/ordering future cache line before needed, avoiding pipeline stalls.

The shown example, that just writes to RAM, is probably just fast enough (it is harder to optimize read cases), although a bit of unrolling could reduce jump penalty impact (specially on non superscalar or in in-order superscalar CPUs -typical ARM included on handheld devices-).

Re: Android Project Changeset 4f8b683: libc/memset.c

#48

Post by tedunangst »

humbledrone wrote: post #36
jmillikin wrote: post #28

Earlier quoted context omitted.

The "unused parameter" error is one of the most useless of C compiler errors. There are dozens of perfectly valid reasons why a parameter would go unused, so the occasional error just gets lost in the noise. It's more helpful in C++, which can omit parameter names to indicate that it's intentionally unused. But then you're using C++; not worth the tradeoff.

It's not hard to type a statement like "(void)v;" to explicitly say, "v is intentionally unused." I fail to see the drawback of this approach. Of course you should only do this when (1) you can't remove the parameter from the function's prototype and (2) there is a good reason for it to be unused. But once you develop that habit, the compiler can help you catch bugs that are otherwise easy to miss. Sounds like a wort…

Then you get a "statement with no effect" warning, which is usually more serious. I'll take a bogus warning in real code over a bogus warning in contrived code any day.

Re: Android Project Changeset 4f8b683: libc/memset.c

#49

Post by joezydeco »

neilc wrote: post #26
pkaler wrote: post #17

Forget the typo. This is the slowest, most naive implementation of memset possible. Is this actual shipping code? I've followed the changeset up to the top and it looks like it's in platform/bootable/bootloader/legacy. Hopefully this isn't actually called anywhere, but the review date says May 13th. What's the point of optimizing the Java VM when functions like memset, malloc, memcmp, memcpy, memchr, etc, etc haven't…

Given that it is in "platform/bootable/bootloader/legacy", I'm not sure there is much point getting all worked up about performance -- presumably something much more efficient would be used for the vast majority of cases.

I haven't updated my source tree in a long time, but the ARM devices were using bionic/libc/arch-arm/bionic/memset.S. This is pure ARM assembly optimized for the pipeline and cache.

My ARM assembly is a little rusty, but it looks like they implement memset and bzero, the latter just calling memset with R1=0.

Re: Android Project Changeset 4f8b683: libc/memset.c

#50

Post by sshumaker »

ars wrote: post #45
tptacek wrote: post #38

Earlier quoted context omitted.

You'd unroll the loop. But knowing the average workload for memset(), it probably wouldn't be a perceptible win.

With todays CPU, particularly with branch prediction, and cache, loop unrolling is not always better. In fact I bet the performance would be worse, especially because you have to worry about setting areas of memory that are not a multiple of the loop unroll size. (I guess you could use duff's device.) I actually tested it while ignoring that, and the unrolled version took 34.551 seconds vs 34.239 for the regular vers…

I would imagine writing the native register size bytes per iteration would be a win, with a little cleanup for the remaining bytes. For a 32-bit architecture, you'd typically be doing 1/4 the iterations.
Post reply on HN