Make Allocator.h less silly (no creepy "proto" object).

[ywkaras]

No description provided.

[SolidWallOfCode]

One of the main points of the class proxy allocator is to use the "proto" object to have better construction performance because a constructor is unlikely to be faster than a memcpy. One could debate whether that's really true, given subsequent calls to init, but it would seem a debate that should be resolved before removing that functionality.

[scw00]

As @SolidWallOfCode talk about, You need cleanup all init function before this pr.

[zwoop]

We've talked about doing these in-place new() (or whatever the C++ nerds calls it). I'm generally ok with rolling forward into modern C++, but, we should definitely

1. Verify performance
2. As amc / scw00 points out, consider restricting critical classes, such that we minimize functionality in init() vs normal initializer.

I imagine if all classes had member initializers in the tor's (and no init), this in-place new() would be roughly the same as our memcpy ? But how expensive are those Init()'s ?

[maskit]

I don't remember the detail, but I faced an issue with init() 1-2 years ago. A std::vector in a class (a member variable) was not initialized. There might be a better way but I think I stopped using alloc-and-init for the class or used placement new after alloc.

[SolidWallOfCode]

Yes, it's definitely a problem to use this with any STL container class (vector, string, etc.).

I suspect that modern compilers, along with direct member initialization and placement new, would make the performance difference negligible and the code much cleaner. It's a big change, however, and we'll probably want to do it incrementally per allocated class, rather than in one big change.

[zwoop]

@SolidWallOfCode So what's the step forward here? Fix classes first, and then do this patch? This is definitely not for v9.x ;).

[ywkaras]

Hmm I think it's far from clear that the proto object leads to overall better performance in ATS. For a class like this:

extern const int ca, cb, cc;
struct S
{
  int a = ca, b = cb, c = cc;
};

I would bet it would. For:

struct S
{
  int a = 1, b = 2, c = 3;
};

it might be faster if the proto object were in L1 cache, otherwise probably not. And for:

struct S
{
  int a, b, c;
};

direct construction corresponds to no object code.

[ywkaras]

@scw00 the behavior of the current code should be the same without moving init() code into the constructor. Why do you say it needs to be done all in one step?

[ywkaras]

@zwoop it already uses placement new, but it's currently only used to make the proto object. I don't know why. It's risky that there's no alignas(C) in front of typeObject. I think we can't know the thinking behind this code now that warpships from the The Planet Inktomi have stopped visiting Earth.

[ywkaras]

I feel like we're a bit random about when we want to block stuff without performance testing. Ideally, a performance regression test would be done on every PR or at least nightly.

[scw00]

Sorry for bother you . I'm fine with this pr, and I'm bother with this case too. My only concern is performance.

IIUC, the original code using ats containers. And it works fine with empty constructor (constructor without args). So we are using one static object and initialize it, then copy it to every new one. The problem is we can not pass args. This is why we have init function. Another problem is destructor, as you see, the ptr only support free function. So your destructor might not work for most of these cases.

So, now you want to remove this performance trick. It would be better to give a convinced evidence to explain which way is best.

Finally, I'd love to approve this pr. So show us how much performance we gain or lost.

[randall]

@ywkaras have you been able to run the autests with this PR locally ?

[ywkaras]

Yes in my build/test environment they all worked. I have to put this aside for a bit I'm working on an xdebug problem we're seeing. I doubt I can satisfy the perf concerns until we have some automated perf tests using traffic replay.

[bryancall]

[approve ci autest]

[randall]

[approve ci autest]

[ywkaras]

[approve ci autest]

[randall]

[approve ci autest]

[randall]

[approve ci autest]