Skip to content

Conversation

redboltz
Copy link
Contributor

@redboltz redboltz mentioned this pull request Nov 11, 2013
@nobu-k
Copy link
Member

nobu-k commented Dec 16, 2013

I tested this code and #43 with gcc in some Linux distributions and clang in Mavericks. For gcc, it was indeed same or even faster to use memcpy instead of simple assignments. However, for clang, the original was 10% faster than the version using memcpy.

Since the performance of this part of code is very critical, it might be better to provide two implementations for gcc and clang (or other compilers) separately. What do you think, @redboltz ?

@redboltz
Copy link
Contributor Author

Thanks for testing it. I agree with you. This modification is original from a workaround for gcc's bug.
So, I think that the following approach is good.

# if defined(__GNUC__)
memcpy version
#else 
assign version
#endif 

@nobu-k
Copy link
Member

nobu-k commented Dec 16, 2013

Thanks. It seems good. Could you add it to this pull request and merge #38 and #43?

@redboltz
Copy link
Contributor Author

I realized that GNUC is defined both gcc and clang. We need to find another approach.

…rator.

The memcpy approach is a workaround for gcc's bug. The speed performance on gcc is the same between both approach. But on clang, the memcpy approach is 10% slower than an assignment approach. Hence I added the switching approach code using compiler checked macro.

Note: __GNUC__ is defined both gcc and clang. So I use __GNUC__ && !__clang__
redboltz added a commit that referenced this pull request Dec 16, 2013
@redboltz redboltz merged commit c04ef9e into msgpack:master Dec 16, 2013
@redboltz redboltz deleted the copy_msgpack_object_by_memcpy branch March 31, 2014 08:56
redboltz added a commit to redboltz/msgpack-c that referenced this pull request Jun 22, 2014
redboltz added a commit to redboltz/msgpack-c that referenced this pull request Jul 11, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants