Get rid of WildBag class

[apotapov]

It's a bad idea to have this class for a number of reasons. The implementation is not very clean, it exposes dangerous functionality to the user of the class. Having this "internal" is not a very good excuse. It promotes bad coding practices inside the library.

My suggestion would be to change the clear method of the Bag class to be:

    public void clear() {
        Arrays.fill(data, null);
        size = 0;
    }

And also add a new method along the lines of:

    public void clearNonFreeing() {
        size = 0;
    }

[lopho]

public void clearNonFreeing() {
    size = 0;
}

This would be even more dangerous, as it is now public and usable from everywhere when directly in Bag.

clear() was this way before. I did extensive profiling on it and generating a new array via data = new Object[size]; yielded an 80% speed increase compared to Arrays.fill As fill is actually a loop iterating over the array and filling it with null, whereas new bulk fills the new array via a native call (memcpy or the likes). Inlining eliminates much of the overhead of course, but it was still very noticable.

See #6

This could be not very memory friendly, but the memory profiling did not show any excessive allocating and collecting (on my machine). But the grain of salt is, that my testing was done only on desktop, not Andriod/Dalvik, where it could behave a lot different.

[apotapov]

If the method is well documented, I don't think it will be very dangerous, especially since the behavior is pretty clearly defined by the method signature. I think this would be a useful piece of functionality to anyone using a Bag class.

As for memory allocation, I am thinking from the perspective of Android/Dalvik. And I'm going off: http://developer.android.com/training/articles/perf-tips.html

The recommendation there is not to allocate memory unnecessarily. However, I haven't tried to benchmark this myself.

[lopho]

Hm, I'm still not convinced by the method name. This implies that some form of clearing is taking place, but it is nothing more but setting the size to zero. If I think "fuck memory, I don't need freeing, pff." and then use the clearNonFreeing() expecting to clear my bag, then put ONE entity in, remove the entity again, and in data[1] there still was an entitiy lingering, bag would fill data[0] with the seemingly "cleared" entity.

I think this method should be clearly labeled as what it really does, and warn about it in the doc.

[lopho]

I have a pull request waiting though, implementing it exactly the way you mentioned. (Except for the new array thing). https://github.com/lopho/artemis-odb/commit/ea5ba5f450bc917536fd8f32bbee1286fb14a6a7

[junkdog]

Hmm..

I'm more in favor of keeping the WildBag as it is. The Bag class is already a bit of a hack (ok, maybe primarily from adding #getData, but its quirky mishmashing of container types is confusing in and of itself). I'd think there would be more appropriate containers provided by whichever framework artemis is used in (like, libgdx's Array).

WildBag already communicates that the one should be cautious using the class, and the limited scope is easy to survey, It might not be the most elegant solution, but its relatively safe (from abuse) and faster. There's already stuff like DumbUnsafeIntArray to speed up execution.

Ideally, I think I'd prefer: Bag#clear - null-fill from index to size. Bag#fastClear - newing the backing array WildBag#setReportedSize or WildBag#unsafeSetSize

[junkdog]

About Bag#fastClear, semantically fast already implies there's some caveat using the method, else there wouldn't be a need for a separate method - which is apparently faster yet not conventionally named (ie Bag#clear).

[lopho]

I think we could even drop the Bag#fastClear and go with only Bag#clear with the null-fill as Bag#clear as it is handled currently, (getData -> read -> null), clear is only called ONCE per World#process (when clearing the World#deleted bag (instead of the old 5 times).

So, currently, we could actually do without

[apotapov]

Right. I think the DumbUnsafeIntArray should be removed as well. I decided not to pile all my qualms at once. :)

The thing that I'm worried about is that this opens a can of worms for other similar hacks in the future, where you can site the presence of WildBag and DumbArray as justification.

We should strive for both clean API and implementation. I think that the functionality WildBag provides is useful, it just needs to be packaged a little bit nicer and made available to the user.

As for replacing the Bag entirely, it might make sense. Not sure if introducing a libgdx dependency is the way to go though. (I probably shouldn't be the one to talk :)

But coming back to the issue at hand, I don't think it make sense to allow setting the size explicitly on the Bag. That just has "abuse me" written all over it. Plus it won't be clear (no pun) to the user why that method is available.

The essence of the operation (or at least how it's currently used) is to clear out the Bag. Not sure what the method signature would be. But as @junkdog mentioned the fact that it will have a non-conventional name will imply a special use case.

[junkdog]

I think we could even drop the Bag#fastClear and go with only Bag#clear with the null-fill as Bag#clear as it is handled currently, (getData -> read -> null), clear is only called ONCE per World#process (when clearing the World#deleted bag (instead of the old 5 times).

This is actually very true, probably the cleanest way to go.

Right. I think the DumbUnsafeIntArray should be removed as well. I decided not to pile all my qualms at once. :)

The thing that I'm worried about is that this opens a can of worms for other similar hacks in the future, where you can site the presence of WildBag and DumbArray as justification.

We should strive for both clean API and implementation. I think that the functionality WildBag provides is useful, it just needs to be packaged a little bit nicer and made available to the user.

I think some hack-ish filth can happily hide behind the veil as long as it speeds up execution or reduces memory footprint without opening up something suspect to the user - and without compromising readability. WildBag I think should stay hidden, as it has side-effects that aren't apparent without inspecting the code (this is true for Bag too: getData, set, clear - even if we have javadocs now) - adding set(Unsafe)Size to the mix would just make it even more complicated.

I thought about pulling in libgdx's IntArray, but in the end decided that it was just too much code for a very limited scope. I don't think Artemis is well suited as a framework for gamedev-optimized containers and such: I want to keep Artemis clean too, but my own emphasis has been on keeping it clean in functionality and retaining the simplicity of use; tried avoiding sprinkling in too much functionality that isn't intrinsic to entity systems (hence, in my mind, ~20LOC DumbArray is cleaner than a few hundred LOC from IntArray).