{"action":"created","issue":{"url":"https://api.github.com/repos/dotnet/coreclr/issues/15025","repository_url":"https://api.github.com/repos/dotnet/coreclr","labels_url":"https://api.github.com/repos/dotnet/coreclr/issues/15025/labels{/name}","comments_url":"https://api.github.com/repos/dotnet/coreclr/issues/15025/comments","events_url":"https://api.github.com/repos/dotnet/coreclr/issues/15025/events","html_url":"https://github.com/dotnet/coreclr/pull/15025","id":273917531,"node_id":"MDExOlB1bGxSZXF1ZXN0MTUyNTk4NzU4","number":15025,"title":"Stop passing CompAllocator pointers around","user":{"login":"mikedn","id":9973420,"node_id":"MDQ6VXNlcjk5NzM0MjA=","avatar_url":"https://avatars2.githubusercontent.com/u/9973420?v=4","gravatar_id":"","url":"https://api.github.com/users/mikedn","html_url":"https://github.com/mikedn","followers_url":"https://api.github.com/users/mikedn/followers","following_url":"https://api.github.com/users/mikedn/following{/other_user}","gists_url":"https://api.github.com/users/mikedn/gists{/gist_id}","starred_url":"https://api.github.com/users/mikedn/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mikedn/subscriptions","organizations_url":"https://api.github.com/users/mikedn/orgs","repos_url":"https://api.github.com/users/mikedn/repos","events_url":"https://api.github.com/users/mikedn/events{/privacy}","received_events_url":"https://api.github.com/users/mikedn/received_events","type":"User","site_admin":false},"labels":[{"id":196940911,"node_id":"MDU6TGFiZWwxOTY5NDA5MTE=","url":"https://api.github.com/repos/dotnet/coreclr/labels/area-CodeGen","name":"area-CodeGen","color":"d4c5f9","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":{"url":"https://api.github.com/repos/dotnet/coreclr/milestones/18","html_url":"https://github.com/dotnet/coreclr/milestone/18","labels_url":"https://api.github.com/repos/dotnet/coreclr/milestones/18/labels","id":3246413,"node_id":"MDk6TWlsZXN0b25lMzI0NjQxMw==","number":18,"title":"3.0","description":".NET Core 3.0, next major release after 2.1. See [official release dates](https://github.com/dotnet/core/blob/master/roadmap.md) for details.\r\n\r\nWe will move important issues from Future to 3.0 as the priorities and dates for next release become more clear.","creator":{"login":"karelz","id":5776068,"node_id":"MDQ6VXNlcjU3NzYwNjg=","avatar_url":"https://avatars2.githubusercontent.com/u/5776068?v=4","gravatar_id":"","url":"https://api.github.com/users/karelz","html_url":"https://github.com/karelz","followers_url":"https://api.github.com/users/karelz/followers","following_url":"https://api.github.com/users/karelz/following{/other_user}","gists_url":"https://api.github.com/users/karelz/gists{/gist_id}","starred_url":"https://api.github.com/users/karelz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/karelz/subscriptions","organizations_url":"https://api.github.com/users/karelz/orgs","repos_url":"https://api.github.com/users/karelz/repos","events_url":"https://api.github.com/users/karelz/events{/privacy}","received_events_url":"https://api.github.com/users/karelz/received_events","type":"User","site_admin":false},"open_issues":122,"closed_issues":90,"state":"open","created_at":"2018-04-05T22:09:22Z","updated_at":"2018-06-27T21:16:51Z","due_on":null,"closed_at":null},"comments":10,"created_at":"2017-11-14T19:41:09Z","updated_at":"2018-06-28T00:04:40Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/dotnet/coreclr/pulls/15025","html_url":"https://github.com/dotnet/coreclr/pull/15025","diff_url":"https://github.com/dotnet/coreclr/pull/15025.diff","patch_url":"https://github.com/dotnet/coreclr/pull/15025.patch"},"body":"Currently `CompAllocator` is passed around by pointer even though (when `MEASURE_MEM_ALLOC` is not defined) it is a pointer sized object and thus it would make more sense to pass around by value. This is problematic because:\r\n* It tends to require the allocator to be dynamically allocated. It is possible to avoid dynamic allocation (e.g. as in `optVnCopyProp`) but this needs care to avoid creating objects that depend on the allocator but have a longer lifetime. Now, dynamic allocation wouldn't be so bad in itself but if you do allocate dynamically you need to cache the allocator as well (e.g. `LinearScan::getAllocator`). In all, it's a bit of hassle to use `CompAllocator` and I suspect that this is one reason why quite a bit of JIT memory ends up in `CMK_Generic`/`Unknown` or other \"general purpose\" buckets.\r\n* It introduces extra indirections - `jitstd::list` contains a pointer to a `CompAllocator` which contains a pointer to a `Compiler` which contains a pointer to an `ArenaAllocator`. Out of 8 instructions required for an allocation 2 are wasted following through indirections.\r\n\r\nOne problem with passing the existing `CompAllocator` around by value is that when `MEASURE_MEM_ALLOC` is enabled the size of the `CompAllocator` increases and together with the size of any objects that contains an allocator object (e.g. `jitstd::vector`). This will end up skewing memory statistics somewhat.\r\n\r\nThe solution is to always make `CompAllocator` pointer sized. When `MEASURE_MEM_ALLOC` is not enabled it should contain a pointer to `ArenaAllocator`. When `MEASURE_MEM_ALLOC` is enabled it should contain a pointer to an `ArenaAllocator` wrapper object that holds a pointer to the real arena and the memory kind value. There would be one such wrapper object per memory kind value.\r\n\r\nInitially I tried to handle this in a more surgical manner but things got a bit ugly - `CompAllocator` relied on `Compiler::compGetMem` and it was `compGetMem` that took care of tracking memory stats when it's `ArenaAllocator` that should be doing that. So I ended up moving `MemStats` to `ArenaAllocator`.\r\n\r\nAnd then moving mem stats to arena got complicated too due to `PooledAllocator`. Its existence meant that memory stats had to be reset every time the pooled allocator was reused. But the whole idea of a pooled allocator doesn't really make sense, you can't and don't need to pool the allocator because pretty much it's entire state needs to be reset anyway. What needs to be pooled is just a 64k memory page. So no more `PooledAllocator`, an `ArenaAllocator` is always created when a method is compiled and on first allocation request it will attempt to grab the pooled page if available.\r\n\r\nSince changes around `CompAllocator` already affected various allocation sites I used the opportunity to do a bit of cleanup:\r\n* `CompAllocator`'s allocation interface was a leftover from the days of `IAllocator` and it's rather clumsy. I changed it to look more like `std::allocator` interface - a single `allocate` function that takes a `count` parameter that indicates how many objects to allocate. Avoids the need for `sizeof(X)` and cast from `void*` to `X*`. It's not exactly like `std::allocator`, that one is a template and I see no reason to mirror that.\r\n* Got rid of `compGetMem` and `compGetMemArray`. There weren't too many uses of these functions and they're just redundant.\r\n* Changed `SmallHashTable` to use `CompAllocator` like other JIT collection classes do. It was using a special, global wrapper of `Compiler::compGetMem`.\r\n\r\nThere's one small change related `compGetMemArray` that's worth mentioning - the integer overflow check. The old code did `numElem > (MAX_MEMORY_PER_ALLOCATION / elemSize)` where `MAX_MEMORY_PER_ALLOCATION` is 512 megabytes, this not only checked for integer overflow but also capped the allocation size to 512MB. The new code uses `SIZE_MAX`, it's faster on 64bit targets because in many cases the number of elements is `unsigned` and the C++ compiler eliminates the entire check. But this version does not limit the allocation size, if this is considered useful (e.g. it may stop the JIT before consuming all available memory) then such a check should be added to `ArenaAllocator::allocateNewPage`, where it has minimal perf impact.\r\n\r\nPIN shows a 0.15% improvement in instructions retired: https://1drv.ms/x/s!Av4baJYSo5pjgr1zpvRR2DPBIP_RSA"},"comment":{"url":"https://api.github.com/repos/dotnet/coreclr/issues/comments/400868230","html_url":"https://github.com/dotnet/coreclr/pull/15025#issuecomment-400868230","issue_url":"https://api.github.com/repos/dotnet/coreclr/issues/15025","id":400868230,"node_id":"MDEyOklzc3VlQ29tbWVudDQwMDg2ODIzMA==","user":{"login":"AndyAyersMS","id":10121823,"node_id":"MDQ6VXNlcjEwMTIxODIz","avatar_url":"https://avatars1.githubusercontent.com/u/10121823?v=4","gravatar_id":"","url":"https://api.github.com/users/AndyAyersMS","html_url":"https://github.com/AndyAyersMS","followers_url":"https://api.github.com/users/AndyAyersMS/followers","following_url":"https://api.github.com/users/AndyAyersMS/following{/other_user}","gists_url":"https://api.github.com/users/AndyAyersMS/gists{/gist_id}","starred_url":"https://api.github.com/users/AndyAyersMS/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/AndyAyersMS/subscriptions","organizations_url":"https://api.github.com/users/AndyAyersMS/orgs","repos_url":"https://api.github.com/users/AndyAyersMS/repos","events_url":"https://api.github.com/users/AndyAyersMS/events{/privacy}","received_events_url":"https://api.github.com/users/AndyAyersMS/received_events","type":"User","site_admin":false},"created_at":"2018-06-28T00:04:40Z","updated_at":"2018-06-28T00:04:40Z","author_association":"MEMBER","body":"@mikedn want to squash and add something to the coding conventions doc? Then I think we're set to merge."}}
{"actor":{"display_login":"AndyAyersMS"}}