search

Gamua / Sparrow-Framework

The Open Source Game Engine for iOS
http://www.sparrow-framework.org
Other
294 stars 83 forks source link

SPPoolObject modifications #18

Closed racarone closed 11 years ago

PrimaryFeather commented 11 years ago

Wow ... I must admit there's a bunch of code in there that I don't fully understand g. ;-)

What I could do, though, is create a small benchmark to see how well this works! I was especially nervous about the thread-safe implementation, because I had tried that in the past and it was too slow. I was not surprised, though, that you managed to pull this off!

Here is the benchmark code: https://gist.github.com/PrimaryFeather/7136635

And here are the results; each benchmark was repeated 4 times on my iPhone 4S in release mode:

New implementation - Non-atomic:

892480 objects per second
895972 objects per second
898051 objects per second
891720 objects per second

New implementation - Atomic:

740804 objects per second
790667 objects per second
806089 objects per second
796141 objects per second

Sparrow 2.0.1 - Active Pooling:

799021 objects per second
834526 objects per second
836893 objects per second
834363 objects per second

No pooling:

457059 objects per second
388692 objects per second
444565 objects per second
420748 objects per second

So your non-atomic implementation is super fast, and even the thread-safe implementation is almost as fast as my naive implementation. Both are about twice as fast as the default (no pooling). Great work!!!

I haven't used the atomic queue yet, so I can't comment on that part of the code -- but it looks very smart. ;-)

The way to generate the class key does look a little bit suspicious, though. Is it sure that those 9 bits are always different per class? Or just very, very, very likely?

(Even if it's the latter, it's probably not a problem. I just want to know if there's some logic I don't understand here.)

racarone commented 11 years ago

What I could do, though, is create a small benchmark to see how well this works! I was especially nervous about the thread-safe implementation, because I had tried that in the past and it was too slow. I was not surprised, though, that you managed to pull this off!

I'm glad you benchmarked it, I forgot to. =P I just assumed it would just be faster, but it's nice to actually see the difference! Saving that msgSend to "poolInfo" is most of it I'm sure. I just thought a simple static hash table would be nice since the chances of you having hundreds of classes inherit from SPPoolObject is practically zero.

I haven't used the atomic queue yet, so I can't comment on that part of the code -- but it looks very smart. ;-)

All credit for the queue goes to Apple, it's open source so the non atomic version is just their code without the locks. ;-)

The way to generate the class key does look a little bit suspicious, though. Is it sure that those 9 bits are always different per class? Or just very, very, very likely?

Ya it's just very very unlikely, but you're right - you never know. I did that just because NSObject just returns the pointer for the default hash method and the chances are just so low with such a small number of objects. But I updated it with a hash function that just shifts the pointer right by 2, which is what Apple uses internally for pointers in NSMapTable so that's good enough for me. =)

PrimaryFeather commented 11 years ago

Thanks for the additional infos! Ah, if Apple uses that strategy internally, then it is definitely good enough for us, LOL. ;-)

One last question before I merge: why the underscores in front of the queue method names? Is that a convention you want to add to our (never written down g) styleguide?

I know I can be super annoying, sorry for that! ;-)

racarone commented 11 years ago

One last question before I merge: why the underscores in front of the queue method names? Is that a convention you want to add to our (never written down g) styleguide?

The reason I do that is in the off chance someone named a C function the same anywhere in their own source it would cause a linker error with the library and since it's a "internal" function my habit is to prefix it with an underscore. Which I didn't do for the queue and dequeue functions, which could actually cause a problem. To cite Apple again =P they actually name all their internal C functions and static variables "_XXMyClassMyName" for the same reason, which might not be a bad route either.

I know I can be super annoying, sorry for that! ;-)

Haha, not at all! I'm actually very grateful for the scrutiny, it helps make better code. =)

PrimaryFeather commented 11 years ago

So this happens even if the C function is not exported, i.e. only visible inside this one .m-file? It's been a while since I've done pure C without the "Objective" additions ... ;-)

racarone commented 11 years ago

Exactly and I think there are ways around it, but it's easier just to avoid it by using unique names.

PrimaryFeather commented 11 years ago

OK, now I understand why you changed the naming of those methods in general (in addition to adding the prefix). Makes sense! Thanks for the explanation, and -- again -- thanks for the pull request! =)

ericoe commented 11 years ago

If you put “static” on your c fcs it will munge the name and linker conflicts will be prevented.

racarone commented 11 years ago

If you put “static” on your c fcs it will munge the name and linker conflicts will be prevented.

Ah that is true, but for a library I still feel it's good form.

PrimaryFeather commented 10 years ago

Now that I review this again (sorry for the long delay!), I'd actually prefer if we made those methods static. Then we'd definitely be on the safe side concerning conflicts.

... and we might even give those methods less cryptic names, which would enhance readability.

Or would there be any downsides?

racarone commented 10 years ago

Most of the methods were already static, with the exception of the queue/dequeue functions, so I made those static. I made changes for readability, so let me know if the names are along the lines you wanted. I also removed the memory barriers for the atomic version, since no one should be using 'retainCount' anyways - it's very fast now.

PrimaryFeather commented 10 years ago

Ah, you're right -- sorry, I didn't notice that SP_INLINE also implied "static". Then it's really just the queue methods that are left.

You said you made those changes, but I didn't get any notification from GitHub; could you tell me where to find them? Then I'll merge that in, too.

racarone commented 10 years ago

Sorry made a new pull request!