feat: fix oom bug

[georgehao]

fix issue #268

[0xmountaintop]

@mask-pp uses sync.Pool to reduce GC pressure? Is that still the case?

related PRs:

• fix bug and optimize fill_trace logic for gc (#104) #104
• Upgrade trace to decrease GC pressure. #92
• refactor: optimize and improve tps for contract call. #83

[colinlyguo]

LGTM

[colinlyguo]

@mask-pp uses sync.Pool to reduce GC pressure? Is that still the case?

related PRs:

• fix bug and optimize fill_trace logic for gc (#104) #104
• Upgrade trace to decrease GC pressure. #92
• refactor: optimize and improve tps for contract call. #83

@georgehao mentioned that it is due to the use of runtime.SetFinalizer, what was the reason for using it in the first place? And could we replace runtime.SetFinalizer with simple loggerResPool.Put(logRes)?

[mask-pp]

In fact, use sync.Pool is a trade-off between memory resources and computing resources.
This way will increase gc pressure, but I'm not sure how much it will be affected, so making a pressure test to support the view is necessary.

[0xmountaintop]

In fact, use sync.Pool is a trade-off between memory resources and computing resources. This way will increase gc pressure, but I'm not sure how much it will be affected, so making a pressure test to support the view is necessary.

@georgehao can you do a benchmarking on CPU?

[georgehao]

In fact, use sync.Pool is a trade-off between memory resources and computing resources. This way will increase gc pressure, but I'm not sure how much it will be affected, so making a pressure test to support the view is necessary.

I can understand your purpose to add sync.Pool for the structLog. But the way your add is not correct. this doesn't repeated
use the memory and may let memory can't be clean with gc.

[georgehao]

you will see all the memory all held with sync.Pool.

[georgehao]

In fact, use sync.Pool is a trade-off between memory resources and computing resources. This way will increase gc pressure, but I'm not sure how much it will be affected, so making a pressure test to support the view is necessary.

@georgehao can you do a benchmarking on CPU?

this image is i benchmark the scroll_getBlockTraceByNumberOrHash in staging, the usage cpu. the sync.Pool used can't resolve it.

I benchmark the scroll_getBlockTraceByNumberOrHash, get the flame graph. i found the high usage of cpu is not called by gc.

i will resolve the high usage cpu these days.

[colinlyguo]

you will see all the memory all held with sync.Pool.

Would only using loggerResPool.Put(logRes) resolve this?

[georgehao]

you will see all the memory all held with sync.Pool.

Would only using loggerResPool.Put(logRes) resolve this?

add sync.pool for structLog have a difficult. because the structLog is store in the array.

[georgehao]

sync.Pool always resolve the CPU usage by gc. but currently, i analysis the flame graph, the cpu usage by gc is a little(5%). so use sync.Pool effect is not obvious. just remove it.

this flame graph is capture from the staging environment.

[0xmountaintop]

after updating https://github.com/scroll-tech/go-ethereum/blob/fix/debug_traceTransaction/params/version.go#L27 to 6 (so that version is 3.1.6) I think we can merge this

[colinlyguo]

you will see all the memory all held with sync.Pool.

Would only using loggerResPool.Put(logRes) resolve this?

add sync.pool for structLog have a difficult. because the structLog is store in the array.

Thanks, I see now.