core/rawdb/freezer: subtract size of hidden items from AncientSize() #27950
Conversation
…zerTable core/rawdb/freezer: sizeHidden should be subtracted from Freezer.AncientSize
karloku
requested review from
karalabe,
holiman and
rjl493456442
as code owners
core/rawdb/freezer_table.go
Outdated
| return t.sizeHiddenNoLock() | ||
| } | ||
|
|
||
| // sizeHiddenNoLock returns the total data size of hidden items in the freezer table | ||
| // without obtaining the mutex first. | ||
| func (t *freezerTable) sizeHiddenNoLock() (uint64, error) { |
There was a problem hiding this comment.
| return t.sizeHiddenNoLock() | |
| } | |
| // sizeHiddenNoLock returns the total data size of hidden items in the freezer table | |
| // without obtaining the mutex first. | |
| func (t *freezerTable) sizeHiddenNoLock() (uint64, error) { |
Haven't looked at the correctness of the implementation yet, but this stood out to me. Why even have the NoLock method when we're not using it - it just makes the api surface more complicated IMO.
There was a problem hiding this comment.
I saw the sizeNoLock function and thought there was the design to leave the NoLock version. Yes, it was not necessary in this change. I would remove it later.
|
Note, a similar change was already submitted in #27700 |
core/rawdb/freezer_table.go
Outdated
| return 0, nil | ||
| } | ||
|
|
||
| indices, err := t.getIndices(offsetItems, hiddenItems-offsetItems) |
There was a problem hiding this comment.
You don't really need to resolve all the hidden items and calculate the total hidden data size.
Instead, you can just retrieve the offset corresponds to the first non-hidden item, and the
offset is actually the hidden data size.
// sizeHidden returns the total data size of hidden items in the freezer table.
func (t *freezerTable) sizeHidden() (uint64, error) {
t.lock.RLock()
defer t.lock.RUnlock()
hidden, offset := t.itemHidden.Load(), t.itemOffset.Load()
if hidden <= offset {
return 0, nil
}
indices, err := t.getIndices(hidden, 1)
if err != nil {
return 0, err
}
return uint64(indices[0].offset), nil
}There was a problem hiding this comment.
Thank you for the advice! I will try this method.
It seems my test cases failed on Windows environments in AppVeyor builds. The files were not dropped when there were only hidden items after truncation. Is it by design or a bug? It might fail this method.
There was a problem hiding this comment.
Thank you for the advice! I will try this method.
It seems my test cases failed on Windows environments in AppVeyor builds. The files were not dropped when there were only hidden items after truncation. Is it by design or a bug? It might fail this method.
The truncateTail call itself failed in the first place with rename C:\Users\appveyor\AppData\Local\Temp\1\336933468 C:\Users\appveyor\AppData\Local\Temp\1\truncate-tail-12670623878883326278.ridx: Access is denied. https://ci.appveyor.com/project/karloku/go-ethereum/builds/47842081/job/tlckukoxk9mi3j7a#L969
I will try to fix it.
core/rawdb/freezer_table_test.go
Outdated
| @@ -662,87 +662,92 @@ func TestTruncateTail(t *testing.T) { | |||
| t.Parallel() | |||
| rm, wm, sg := metrics.NewMeter(), metrics.NewMeter(), metrics.NewGauge() | |||
| fname := fmt.Sprintf("truncate-tail-%d", rand.Uint64()) | |||
| var maxFileSize uint32 = 50 | |||
There was a problem hiding this comment.
Any reason for changing the file size? I don't think it should be changed just for adding a new non-hidden-size check?
There was a problem hiding this comment.
No, it is just for checking the hidden size. I would change the data sizes instead.
…he sizeHidden Signed-off-by: Karloku Sang <[email protected]>
…ail on Windows Signed-off-by: Karloku Sang <[email protected]>
|
Closed it because #28525 |
This PR changes how
Freezer.AncientSize()reports the total data size, as the issue addressed in #27483 .It introduces two changes:
freezerTable.sizeHidden()return the total data size of the hidden items by summing up the sizes from the hiddenindexEntrys (indices fromitemOffsettoitemHidden).Freezer.AncientSize()is nowsize - sizeHiddento get the actual size of the live items.