Open Bug 1518214 Opened 10 months ago Updated 9 months ago

Improve js::jit::AssemblerBufferWithConstantPools::hasSpaceForInsts performance

Categories

(Core :: JavaScript Engine: JIT, enhancement, P3)

ARM64
Unspecified
enhancement

Tracking

()

People

(Reporter: nbp, Unassigned)

References

(Blocks 2 open bugs)

Details

(Keywords: perf, Whiteboard: [arm64:m4])

The code for s::jit::AssemblerBufferWithConstantPools::hasSpaceForInsts is being executed every time we call vixl::MozBaseAssembler::nextInstrOffset, which implies that we do it for every instruction.

The problem is that this logic is quite complex and adds a lot of code to be executed for each instruction. I suspect that we could simplify this logic by computing ahead the offset at which a pool should be inserted to satisfy all the registered constants.

Therefore replacing all the math of hasSpaceForInsts and moving them to the point where a constant is registered in the constant pool. At the end hasSpaceForInsts should look something along the lines of:

  inline bool hasSpaceForInsts(size_t numInsts) {
      return nextOffset + numInsts * InstSize < nextPoolOffset;
  }

Additionally, we should also move largest/slow functions of AssemblerBufferWithConstantPools to some cpp files.

For ARM I implemented several optimizations to avoid put-into-buffer overhead, see comments above putInt() in IonAssemblerBufferWithConstantPools.h. Subsequently, assembly time on ARM was not much of a factor any more, and profiling did not show hasSpaceForInsts() to take significant time on that platform.

It is possible that the situation is different on ARM64 for other reasons, or that hasSpaceForInsts() has since grown more complex again and takes more time now, or that I did poor profiling work, but it would be nice to have profiling data to support this bug.

As I wrote in bug 1443082, the vixl assembler is (based on profiling evidence anyway, and for webassembly compilation) a very poor performer, and optimizing that first might be important to get meaningful profiles for hasSpaceForInsts().

P3 until we get profiling data to back this guess.

Priority: -- → P3

[arm64:m4] because this bug is not a blocker for ARM64 Fennec riding the trains.

Whiteboard: [arm64:m3] → [arm64:m4]
You need to log in before you can comment on or make changes to this bug.