Allocate space for locals together with stack #382
Conversation
Codecov Report
@@ Coverage Diff @@
## master #382 +/- ##
=======================================
Coverage 98.73% 98.74%
=======================================
Files 58 58
Lines 8703 8765 +62
=======================================
+ Hits 8593 8655 +62
Misses 110 110 |
|
This seems to be very similar to #358. What is the story with this? |
chfast
force-pushed
the
locals_optimization
branch
2 times, most recently
from
cd4815e to
c28b01f
Compare
|
I resurrected this one because I will need it in near future. |
|
LTO builds looks a bit better: |
gumb0
left a comment
gumb0
left a comment
There was a problem hiding this comment.
Looks good overall, would be nice to add some tests with non-empty locals and args.
|
Does it make sense to try to increase |
I tried with 1K and 2K sizes. The performance is not much better, but it causes stack overflow in some infinite call tests. |
chfast
force-pushed
the
locals_optimization
branch
2 times, most recently
from
ab6bea2 to
e6917fa
Compare
|
We may consider renaming
In future it will probably be converted to RAII type and cooperate with "thread execution context" e.g. to bump call depth. Proposals:
|
chfast
force-pushed
the
locals_optimization
branch
from
ab79692 to
e6917fa
Compare
I also tried the small storage of 256 bytes, but I see no performance difference. So leaving it at 128. |
chfast
force-pushed
the
locals_optimization
branch
from
e6917fa to
dc13f1d
Compare
gumb0
left a comment
gumb0
left a comment
There was a problem hiding this comment.
Looks good, it would be also nice to test rbegin() / rend() for case with locals.
| { | ||
| constexpr auto max_height = 33; | ||
| OperandStack stack({}, 0, max_height); | ||
| ASSERT_GT(address_diff(&stack, stack.rbegin()), 100) << "not allocated on the heap"; |
There was a problem hiding this comment.
Why is this GT while the others LT? Ah I see heap in the message.
There was a problem hiding this comment.
The 100 is arbitrary.
axic
left a comment
axic
left a comment
There was a problem hiding this comment.
Can you add a test where it tries to pop the stack to access the locals?
chfast
force-pushed
the
locals_optimization
branch
from
dc13f1d to
a8717bc
Compare
|
|
||
| Value* m_bottom; | ||
|
|
||
| Value* m_locals; |
There was a problem hiding this comment.
Would the order of these three manifest speed differences?
There was a problem hiding this comment.
I have corrected this. New version may be slightly faster, but hard to tell for sure as they are very close.
| if (max_stack_height > small_storage_size) | ||
| m_large_storage = std::make_unique<Value[]>(max_stack_height); | ||
| m_top = bottom() - 1; | ||
| const auto num_args = args.size(); |
There was a problem hiding this comment.
Perhaps this one could make sense to be typed as size_t because then we can be sure storage_size_required is size_t? Though I guess size() returns that anyhow.
There was a problem hiding this comment.
This is fine now, but probably should be changed to uint32_t at some point.
chfast
force-pushed
the
locals_optimization
branch
from
ffe7c0e to
a506ea4
Compare
Extended existing tests to have this check. |
chfast
force-pushed
the
locals_optimization
branch
from
8a4e20d to
e24521b
Compare
This move memory management of args and locals to OperandStack where space for all can be allocated in single go.
chfast
force-pushed
the
locals_optimization
branch
from
e24521b to
642359f
Compare
This move memory management of args and locals to OperandStack where
space for all can be allocated in single go.