userDaniel Pcancel
branchrust_reboot/main/unify_pool_freecancel

36 Commits over 31 Days - 0.05cph!

3 Months Ago
Update: Fixing obsolete FreeList usage Tests: none, trivial change
3 Months Ago
Update: Consolidating Facepunch.System.Tests - Cleaned up a bunch of bogus using statements Tests: Ran combined unit tests - they pass.
3 Months Ago
Merge: from main Tests: ran around, gathered, shot, built
3 Months Ago
Merge: from main Tests: Built all modes
3 Months Ago
Merge: from main Tests: built all modes, built small house, cut a tree
3 Months Ago
Update: Replacing new occurances of obsolete Pool API with new Tests: built all configs
3 Months Ago
Merge: from main Also contains updated codegen binaries to conform with new API Tests: built all modes
3 Months Ago
Update: Fixing deprecation warnings from obsolete Pool API use Tests: none, trivial changes
3 Months Ago
Optim: Avoid unboxing in Pool.Free(ListDictionary) Was reported by Clr Heap Allocation Analyzer and I missed it originally. Tests: none, trivial change
3 Months Ago
Merge: from main Tests: built all targets, ran around solo on CraggyIsland
3 Months Ago
Update: Fixing all deprecated uses of Pool since merge from main Tests: built all targets
3 Months Ago
Update: merge from main Tests: built all targets
3 Months Ago
Update: Pool uses warnings instead of errors for deprecated API Motivation: This will prevent causing errors for our modders/existing plugins and send a message that it needs to be updated. I'll remove these deprecated APIs next patch cycle. - Also removed a couple TODO since they're either not viable for now or complete. Tests: none
3 Months Ago
Update: Replacing all calls to Pool.GetList with Get<List<T>> Now that we're cleaning up lists automatically when we call Free or FreeUnamnaged, GetList lost it's use. Replacement was done via regex + added using namepsace statements where missing. This affected ~740 cases. Tests: Built all targets.
3 Months Ago
Update: Marking Pool.GetList as obsolete - Also bringing back list clearing on GetList invokation - to avoid breaking mods suddenly Tests: built all targets
3 Months Ago
Update: Changing ProtoBuff codegen to use Pool.Get<List<T>> This eliminates 680 cases of GetList<T> use, which is about 40%. - CodeGenerator built in debug conf with commit 7d37b927 (FreeList-Depr branch) Tests: built all targets.
3 Months Ago
Update: Review Feedback - Part 3 Renaming FreeUnmanaged primary overload to FreeUnsafe to clearly indicate that it's an undesired API. Tests: built all targets
3 Months Ago
Tests: Review Feedback - Part 2 This covers basic scnearios of Pool usage. Tests: Ran the new unit tests - they pass.
3 Months Ago
Clean: Review Feedback - Part 1 - Make scopes explicit with brackets - Get rid of System namespace prefix (as we declare that we use System's names) - Renamed `deepFree` defaulted arg into `freeElements` - FreeUnmanaged(MemoryStream) no longer sets Position anymore - Commented the API Tests: Built C+S target (as it's trivial build-only changes)
3 Months Ago
Update: Making Pool.Free capable of "deep freeing" Motivation: Since we're getting rid of custom "FreeList" calls, then we should also remove FreeListAndItems, the last custom func in the "Free" set. - Collections-related Free overload now can deep-free - calling Free on all elements of a collection. - Marked FreeListAndItems as obsolete (with error) and piped it over to Free overload - Updated 4 callsites of FreeListAndItems to use Free(T, true) instead Tests: built all targets.
3 Months Ago
Update: Further constraining Pool - Step 3 Now that all usecases of Pool's API have been redirected to new methods, I can further constrain and annotate the API. - Further constrained all container-like Free overloads to only accept ref-types with default ctor and IPooled interface - Marked FreeList<T> as obsolete (with errors) and piped it into FreeUnmanaged overload (this is dangerous/leaky, but I've updated all existing calls to Free/FreeList) - Marked ClearList as obsolete (with errors) - also inlined it's logic at the only callsite in the project - Removed Clear call from GetList, since now we always clear lists via overloads - Removed checks from main Free overload Tests: built all targets. No runtime tests. Ran around on CraggyIsland in 2 player multiplayer, did minor spawning of items, constructions, shooting, player damage.
3 Months Ago
Update: Replacing all cases of Pool.FreeList - non-controversial cases This modifies the remaining(after previous 2 unsafe submissions) set of cases where we had FreeList(292 files) - they all either fall into FreeUnmanaged(don't implement IPooled) or Free(implements IPooled). The replacement has been done by constraining FreeList<T> with T : IPooled first, this revealed ~560 build errors which got fixed with FreeUnmanaged. Then all the remaining FreeLists that weren't producing an error should've been replaced with Free calls. Tests: built all targets in Unity. Ran around on CraggyIsland in 2 player multiplayer (built a tiny house), checked bullet decals spawning/despawning properly.
3 Months Ago
Update: Migration to "unsafe" FreeUnmanaged - Interface Code This CL contains all cases where we call FreeUnmanaged with T : interface - this is dangerous because we don't know whether T is IPooled or not. If/when in the future we make FreeUnmanaged to skip invoking OnPoolEnter, this will lead to leaks/bugs. Tests: Built all targets.
3 Months Ago
Update: Migration to "unsafe" usage of FreeUnmanaged - Generic Code These cases are part of generic code that either provides containers/algos for other parts of logic or part of currently-dead code. They're unsafe because if they start getting used with T : IPooled, then in the future if/when we change Unmanaged to not check IPooled it might leak/bug out. Tests: Built all targets.
3 Months Ago
Update: New CodeGenerator for Pool.FreeUnmanaged<MemoryStream> change * CodeGenerator built with a3cd77f3 from FreeList-Depr branch * This replaces all uses of Pool.FreeMemoryStream in Rust.Data/generated/ProtocolBuffers.Serializer.cs Tests: built all targets in editor.
3 Months Ago
Update: Updating ProtoBuf Codegen to use new Pool API * Includes new debug CodeGenerator binaries(built from FreeList-Depr branch, commit 1874d134). RustProtoBuf will be merged to main once I get closer to completion of teh refactor. * Generated new Rust.Data/generated/*.generated.cs files with no FreeList calls Tests: build tests for all targets
4 Months Ago
Update: Constraining Pool.Free - Step 2 * Adding collection-related FreeUnmanaged overloads for all existing collection-related Free overloads * Putting IPooled constraint on almost-all collection-related Free overloads (skipping List - that'll be next chonky CL) * Changed StreamBuffer overload to be a FreeUnmanaged overload Tests: built all targets in unity
4 Months Ago
New: Adding Facepunch.Tests assembly for unit testing Facepunch libs, with ListExtension tests Motivation: when working on core libraries we gotta have confidence that we won't bork something somewhere by changing core algos&utils. With this test assembly, we have a common place to document behavior of Facepunch lib before we go modify it. Added 64 test cases for ListExtension changes (I couldn't reliably confirm in 3p multiplayer session). Test assembly configured in lean fashion. Tests: built all targets in editor. Ran all existing tests, including new ones being submitted. Observed ~10 failures in NavMesh and TextMeshPro, but new tests pass.
4 Months Ago
Optim: reimplementing List.Compare to use an inplace algorithm Caused by my upcoming changes to Free<HashSet<T>> requiring IPooled(I could've worked around it, but it was better to optimize it). Although I didn't benchmark, this in theory is a straight win - 3 less loops, no need for extra pools, and also fixes a harmless-but-perf bug(remained set had more elements than expected). Tests: Build for Client target. I also need to do runtime validation with a 3player multiplayer session - I ran out of time, will done once I'm back home.
4 Months Ago
Update: Adding Pool.FreeUnmanaged overload for MemoryStream Since Free got changed to accept IPooled only, it allows us to delete a runtime check in the editor env. Tests: build only in editor, all targets
4 Months Ago
Feedback: Replacing bikeshedded emptyArray with Array.Empty<T> Tests: trivial change, so only built Client+Server
4 Months Ago
Update: Constraining Pool.Free - Step one * Primary Pool.Free overload now only accepts IPooled types * Secondary overloads added to work with collections (not restricted to IPooled yet). They all call Clear() on the returned pooled container. * Pool.FreeList now just pipes to one of Pool.Free secondary overloads (proper cleanup will be done later - there's 800 occurances of FreeList usage) * Added Pool.FreeUnmanaged as an escape hatch for types that are in the pool but don't implement IPooled interface. Motivation: If we want to avoid leaking memory and reduce potential of "improperly-recycled" pooled objects, so we need a more controlling API for Pool that allows users to avoid misusing it and calling the wrong API by accident. We'll get there by providing a stricter API that checks for users whether it's legal to use it in a particular way. This means I have to write a bunch of boilerplate (overloads + variations) and clean up all the use cases we have(both the types that might benefit from IPooled and all the API usage points). Tests: built all targets for scripts in editor. Didn't do any runtime testing as there's only 1 safe functional change (clearing of collection on return to pool).
4 Months Ago
Clean: Nuking SimpleList.cs Nothing uses it anymore, and it's superseeded by BufferList. Tests: All targets in editor built succesfully
4 Months Ago
Update: Replacing SimpleArray uses with BufferList. BufferList also avoids allocations when default-constructed. There's 2 reasons for this change: they're functionally the same(with a small change for default BufferList), but BufferList offers more. Also, and the primary reason, it allows me to refer to the type at a lower assembly level (Facepunch) so that I can continue making Pool's API more strict. Tests: checked all "targets" build in Unity, no runtime tests (well, editor runs this code outside of playmode as well, so there's that)
4 Months Ago
Update: Inlining PoolCollection::Fill implementation into Pool In order for us to unify TakeX/FreeX custom methods into a single overload set, we need to unify their constraints. This removes one new T() call from 2 calls in PoolCollectionm which is first step towards removing new() constraint on PoolCollection(and instead preserving it on Pool APIs). Also left a note about thread safety - luckily right now everything is used in a safe manner
4 Months Ago
Simplification: avoid double hash calculation when pushing new elements into the Pool in editor