Can you please explain that why it needs "__sync_synchronize()" in the source code? #1

Activity

1. cloudwu commented on Sep 24, 2013

   

   cloudwu

   on Sep 24, 2013

   Owner

   More actions

   I guess memory barrier between cstring s and s->type is needed. maybe it's unusefu. Should I remove then?
2. wilsonwen commented on Sep 24, 2013

   

   wilsonwen

   on Sep 24, 2013

   Author

   More actions

   I think here's not the situation '__sync_synchronize' needed. No memory ordering need to be guaranteed because you have used lock during interning cstring, which is the only share data in multithread. No where else can change s->type.
   Anyway it affects little, removing it perhaps improve performance a little.
3. cloudwu commented on Sep 24, 2013

   

   cloudwu

   on Sep 24, 2013

   Owner

   More actions

   I mean

   cstring 
   cstring_persist(const char * cstr, size_t sz) {
       cstring s = cstring_clone(cstr, sz);
       if (s->type == 0) {
           s->type = CSTRING_PERMANENT;
           s->ref = 0;
       }
       __sync_synchronize();
       return s;
   }
4. wilsonwen commented on Sep 24, 2013

   

   wilsonwen

   on Sep 24, 2013

   Author

   More actions

   Do you worried about "return s" runs before "if (s->type == 0)" or "cstring_clone"?

   But I can't tell the problem it cause without '__sync_synchronize();'.
5. cloudwu commented on Sep 24, 2013

   

   cloudwu

   on Sep 24, 2013

   Owner

   More actions

   yeah, maybe s was read by flowing cstring_grab() before s->type changed.

   I'm not sure. But I guess I can remove these '__sync_synchronize();'
6. cloudwu

   added a commit that references this issue on Sep 24, 2013

   remove __sync_synchronize #1

   0bd4f18
7. cloudwu

   closed this as completedon Sep 24, 2013
8. liyf001

   added a commit that references this issue on May 4, 2014

   Revert "remove __sync_synchronize cloudwu#1"

   ...

   15d2187