New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RDY] GA_APPEND() and GA_APPEND_VIA_PTR() #830

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@philix
Copy link
Member

philix commented Jun 11, 2014

Using GA_APPEND is easier than:

ga_grow(&ga, 1);
((item_type *)ga.ga_data)[ga.ga_len] = item;
++ga.ga_len;

GA_APPEND_VIA_PTR is used to replace this common pattern:

ga_grow(&ga, 1);
item_type *p = ((item_type *)ga.ga_data) + ga.ga_len;
p->field1 = v1;
p->field2 = v2;
ga.ga_len++;
@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 11, 2014

Either I'm not very awake yet, or the commit message on f6af13f is a bit fishy:

    ga_grow(&ga, 1);
        item_type *p = ((item_type *)ga.ga_data)[ga.ga_len];
        p->field1 = v1;
        p->field2 = v2;
        ga.ga_len++;

You're dereferencing (item_type *)ga.ga_data with the [] operator, which would result in a item_type (no pointer), then assigning it to a pointer.

@philix

This comment has been minimized.

Copy link
Member

philix commented Jun 11, 2014

Either I'm not very awake yet, or the commit message on f6af13f is a bit fishy:

    ga_grow(&ga, 1);
        item_type *p = ((item_type *)ga.ga_data)[ga.ga_len];
        p->field1 = v1;
        p->field2 = v2;
        ga.ga_len++;

You're awake. I was sleepy when I wrote it. It should be ga.data + ga.ga_len.

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 13, 2014

Just to put it all in one place, here's how I see a possible slight alteration of your GA_APPEND and GA_APPEND_VIA_PTR, the most important change being that you could use GA_APPEND_VIA_PTR like this:

//    ga_grow(&ga, 1);
//        item_type *p = ((item_type *)ga.ga_data)[ga.ga_len];
//        p->field1 = v1;
//        p->field2 = v2;
//        ga.ga_len++;

item_type *p = ga_append_via_ptr(ga);
p->field1 = v1;
p->field2 = v2;

// or if you don't trust ga.ga_itemsize:
item_type *p = GA_APPEND_VIA_PTR_V2(item_type, ga);
p->field1 = v1;
p->field2 = v2;

This in garray.h

/// Make room in growing array "gap" for at least "n" items.
/// Tests if the reallocation is really necessary.
///
/// @param gap
/// @param n
static inline void ga_grow(garray_T *gap, int n) FORCE_INLINE
{
  if (gap->ga_maxlen - gap->ga_len >= n) {
    // the garray still has enough space, do nothing
    return;
  }

  ga_do_grow(gap, n);
}

#define GA_APPEND(item_type, gap, item)                                    \
  do {                                                                     \
    ga_grow(gap, 1);                                                       \
    ((item_type *)(gap)->ga_data)[(gap)->ga_len++] = (item);               \
  } while (0)

// version that trusts gap->ga_itemsize, no macro needed
static inline void *ga_append_via_ptr(garray_T *gap)
{
    ga_grow(gap, 1);
    return (char *)gap->ga_data + (gap->ga_itemsize * gap->ga_len++);
}

// version that doesn't trust gap->ga_itemsize
static inline void *ga_append_via_ptr_v2(garray_T *gap, size_t itemsize) FORCE_INLINE
{
    ga_grow(gap, 1);
    return (char *)gap->ga_data + (itemsize * gap->ga_len++);
}
#define GA_APPEND_VIA_PTR_V2(item_type, gap) ga_append_via_ptr_v2(gap, sizeof(item_type))

EDIT: I do have to mentiond though, if you don't trust ga_itemsize, then ga_grow is unlikely to be correct either...

@philix

This comment has been minimized.

Copy link
Member

philix commented Jun 15, 2014

EDIT: I do have to mentiond though, if you don't trust ga_itemsize, then ga_grow is unlikely to be correct either...

As I said: by luck, many appends occur without a reallocation and ga_itemsize can be wrong without breaking anything. The safe route is to replace code with equivalent code and (type *)gap->ga_data + gap->ga_len is not equivalent to some code that relies on ga_itemsize.

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 15, 2014

The safe route is to replace code with equivalent code and (type *)gap->ga_data + gap->ga_len is not equivalent to some code that relies on ga_itemsize.

True, though it might be easy to "rat out" that code now that we have type information by assert-ing with LINE/func info on sizeof(type) != ga_itemsize. Then again, ga_itemsize doesn't exist in kvec and should go the way of the dodo anyways.

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 16, 2014

@ZyX-I What's the preferred way to use function attributes like ALWAYS_INLINE on static inline functions defined in headers? Actually, what's the general way at all?

For the second part I've had the best success rate with the double-declare approach like this: https://github.com/rikusalminen/threedee-simd/blob/master/include/threedee/vector.h#L11-L12

For msvc it seems like doing it in one line is also good.

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 16, 2014

By the way I wonder why the Clang leak checker is not picking up on the same leak that Valgrind is reporting for the python tests. 't would be weird if that code path wasn't excercised during normal vim tests...

philix added some commits Jun 9, 2014

ga_growsize should be >= 1
I know it could be 0 sometimes. Running the tests with
`assert(gap->ga_growsize > 0)` in ga_grow() crashes nvim while running the
tests.

 - Add a setter for ga_growsize that checks whether the value passed is >=1 (log
	 in case it's not)
 - log when ga_grow() tries to use a ga_growsize that's not >=1
 - use GA_EMPTY_INIT_VALUE is many places
Introduce GA_APPEND()
This macro is used to append an element to a growable array. It replaces this
common idiom:

   ga_grow(&ga, 1);
   ((item_type *)ga.ga_data)[ga.ga_len] = item;
   ++ga.ga_len;
Introduce ga_append_via_ptr() and GA_APPEND_VIA_PTR()
Similar to GA_APPEND(). Replaces this pattern:

    ga_grow(&ga, 1);
    item_type *p = ((item_type *)ga.ga_data) + ga.ga_len;
    p->field1 = v1;
    p->field2 = v2;
    ga.ga_len++;
@philix

This comment has been minimized.

Copy link
Member

philix commented Jun 26, 2014

@aktau build is passing. ga_do_grow as another level of indirection was confusing Valgrind. I'm using ga_grow directly now.

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 26, 2014

I looked at it for a second and it seems like you left ga_grow in garray.c, which means we're sure to get a function call, right? We could perhaps solve that problem later if necessary, though. I liked your idea, because most of the time a function call would be unnecesary.

I'm wondering why that builds for you. In my PR (#869), it looks like the static inlines are giving the unit test C parser a bad run for its money. That seemed reasonable to me. At the time I wrote the parser I thought of supporting the current codebase, and that didn't include static inlines in headers. It shouldn't build because it would be passed to the LuaJIT ffi cdef and it would probably not accept static inlines (or so I thought).

Admittedly, I haven't had time for the problem yet. Perhaps I misjudged it.

EDIT: I think I see what might be the difference: I force the inlining with a FUNC_ATTRIBUTE, perhaps that's messing with things. Though now I'm consfused as to why LuaJIT accepts this horse-radish.

@philix

This comment has been minimized.

Copy link
Member

philix commented Jun 26, 2014

I looked at it for a second and it seems like you left ga_grow in garray.c, which means we're sure to get a function call, right? We could perhaps solve that problem later if necessary, though. I liked your idea, because most of the time a function call would be unnecesary.

@aktau Yes. I gave up on the micro optimization. This can be bad if I put GA_APPEND* inside a loop. This will be very rare though and temporary as we move towards kvec/sds.

When can I tag rag this RDY? Any pending objections?

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Jun 26, 2014

@aktau Yes. I gave up on the micro optimization. This can be bad if I put GA_APPEND* inside a loop. This will be very rare though and temporary as we move towards kvec/sds.

Not really, I think the change is good. We need to move to a compact dynamic array like kvec anyway. Loops is where it hurts and when all variable changes are visible to the compiler (such as with kvec), it can reason about when to execute cmp instructions and such like. Nothing feels quite like seeing the compiler generate a nice, compact loop.

LGTM.

@philix philix changed the title [RFC] GA_APPEND() and GA_APPEND_VIA_PTR() [RDY] GA_APPEND() and GA_APPEND_VIA_PTR() Jun 26, 2014

justinmk added a commit that referenced this pull request Jun 30, 2014

@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented Jun 30, 2014

Merged!

@justinmk justinmk closed this Jun 30, 2014

@philix philix deleted the philix:ga_append branch Dec 16, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment