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

81–90 of 93 posts

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

#81

Post by __david__ »

signa11 wrote: post #70
__david__ wrote: post #58

Earlier quoted context omitted.

Because in boot code you generally don't care about speed, but size is a very important factor. I've personally had to rewrite standard library functions to be intentionally small and stupid just so a piece of boot code could fit into ROM. It's not fun, but there is a certain satisfaction when you rewrite putchar and think, I just saved 10k of code!

> Because in boot code you generally don't care about speed, but size is a very important factor. why can't you have a '#ifdef PREFER_SIZE_OVER_SPEED' or something similar ?

Practicality. Generally your stdlib is part of your cross compiler install. So now you'd need 2 versions of your stdlib--the small version and the speedy version. So now you aren't using -lc (which is usually built-in to the linker) foro your boot code but you are for your regular firmware. Now your makefile is more complex. And you are running a modified std library so it's a pain to upgrade. Blah blah blah the list goes on.

Of course you could do all that, but it's so much easier to just override the 5 functions you actually use in your code. Because of the way linkers work and the way standard libraries are designed, if you define your own version of a library function then the linker won't pull in the library version--effectively choosing your local version over the one in the library. That keeps the standard library clean of patches and keeps the small, tight code near the project that actually needs it.

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

#82

Post by scott_s »

SamReidHughes wrote: post #75
scott_s wrote: post #56

Earlier quoted context omitted.

If I understand you correctly, you think programmers should say: int i; for (i = 0; i Instead of: memset(buffer, 0, SIZE) Because it indicates better buffer management.

If I understand hoelle's point, it is that you shouldn't need to fill the buffer with a constant value in the first place. Why do people do it? The only time you generally need to prefill something is if you're using some algorithm whose initial state includes a prefilled array. It's hard to find a place where a byte array needs to be memset. If you need an array zeroed on first use, why didn't you use calloc? The on…

Sometimes you manage your own memory, so you can't use calloc. Sometimes you're zeroing out stack allocated memory, such as the common practice when doing socket programming in C.

Setting memory you own to a known value is just good, defensive programming. If I screw up - and in C, you're going to screw up - it's good to see a known value rather than unknown values.

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

#83

Post by JoeAltmaier »

__david__ wrote: post #81
signa11 wrote: post #70

Earlier quoted context omitted.

> Because in boot code you generally don't care about speed, but size is a very important factor. why can't you have a '#ifdef PREFER_SIZE_OVER_SPEED' or something similar ?

Practicality. Generally your stdlib is part of your cross compiler install. So now you'd need 2 versions of your stdlib--the small version and the speedy version. So now you aren't using -lc (which is usually built-in to the linker) foro your boot code but you are for your regular firmware. Now your makefile is more complex. And you are running a modified std library so it's a pain to upgrade. Blah blah blah the list…

Compilers have long ago memorized the code for memset (and memcopy and ...) and substitute optimized algorithms. As part of faking their benchmark stats for marketing purposes. So no worries, this code should result in kick-butt optimized assembler. Except for the obvious bug.

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

#84

Post by JoeAltmaier »

sshumaker wrote: post #50
ars wrote: post #45

Earlier quoted context omitted.

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.

Far more involved than that. There are alignment issues. Its several machine cycles faster to do aligned store of a large scalar e.g. 32-bit. So a responsible implementation would do a head check for alignment, store 1,2 or 3 bytes efficiently, then do aligned stores of 32-bit values (or 64-bit or whatever native size is appropriate), then a tail check for the small change. OR better yet switch(p & 3) and handle each case with unrolled code.

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

#85

Post by rbanffy »

ars wrote: post #33
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…

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

I would build it into the memory controller. Better: in the memory itself.

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

#86

Post by tptacek »

ehnus wrote: post #53
tptacek wrote: post #44

Earlier quoted context omitted.

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.

If assertions disappear in certain optimized builds then what do you do, or recommend, to deal with warnings related to unused parameters or values if you don't believe that casting to void has value?

I don't understand how the question you ask even follows from the premise you forwarded. The fact that memset is probably always dead code here is why it's not really a bug. The fact that warnings about unused return values are useless 100-1000 times for every time they're potentially useful is why I don't litter my code with pointless casts.

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

#87

Post by hoelle »

scott_s wrote: post #82
SamReidHughes wrote: post #75

Earlier quoted context omitted.

If I understand hoelle's point, it is that you shouldn't need to fill the buffer with a constant value in the first place. Why do people do it? The only time you generally need to prefill something is if you're using some algorithm whose initial state includes a prefilled array. It's hard to find a place where a byte array needs to be memset. If you need an array zeroed on first use, why didn't you use calloc? The on…

Sometimes you manage your own memory, so you can't use calloc. Sometimes you're zeroing out stack allocated memory, such as the common practice when doing socket programming in C. Setting memory you own to a known value is just good, defensive programming. If I screw up - and in C, you're going to screw up - it's good to see a known value rather than unknown values.

Removing the memsets from the networking layer of one of my projects basically halved the frametime of that system (which was a bottleneck).

I'm all for defensive programming, but when you need to be fast, you'll just have to make sure it's correct.

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

#88

Post by dfox »

rbanffy wrote: post #85
ars wrote: post #33

Earlier quoted context omitted.

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

I would build it into the memory controller. Better: in the memory itself.

Many modern platforms contain some logic, that can be (ab)used to do memset() directly in hardware without processor involvement (ie. in parallel with other code). But this tends to require so much setup and IO overhead so it simply isn't worthwhile to do.

Integrating memset/bzero logic into memory array itself will increase it's price drastically (but in some special cases it is done, generally when memory array already contains some other expensive non-memory logic).

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

#89

Post by dfox »

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…

Don't underestimate the compiler optimizations. GCC 4.3.1 on i386 with -O3 generates pretty involved optimized code that actually stores whole aligned 32bit words. (and I strongly suspect that this generated code is faster than rep stosb).

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

#90

Post by [deleted] »

jmillikin wrote: post #28
humbledrone wrote: post #9

The thing that shocks me about this bug is that it should have been caught when the compiler issued a warning about an unused function parameter. Either the warning was not enabled, or the programmer just ignored it; either of those options indicates a pretty severe lack of professionalism.

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.

[deleted]
Post reply on HN