engine: Support reading objects without decoding by cthulhu-rider · Pull Request #2753 · nspcc-dev/neofs-node

cthulhu-rider · 2024-02-26T15:00:06Z

No description provided.

codecov · 2024-02-26T15:05:44Z

Codecov Report

Attention: Patch coverage is 79.05759% with 40 lines in your changes are missing coverage. Please review.

Project coverage is 22.03%. Comparing base (2da3676) to head (09e385c).

Files	Patch %	Lines
pkg/local_object_storage/blobstor/fstree/fstree.go	44.44%	11 Missing and 4 partials ⚠️
pkg/local_object_storage/blobstor/peapod/peapod.go	66.66%	6 Missing and 1 partial ⚠️
cmd/blobovnicza-to-peapod/blobovniczatree/get.go	0.00%	5 Missing ⚠️
...al_object_storage/blobstor/compression/compress.go	0.00%	5 Missing ⚠️
pkg/local_object_storage/engine/get.go	89.18%	2 Missing and 2 partials ⚠️
pkg/local_object_storage/shard/get.go	95.34%	2 Missing ⚠️
pkg/local_object_storage/writecache/get.go	87.50%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2753      +/-   ##
==========================================
+ Coverage   21.89%   22.03%   +0.14%     
==========================================
  Files         787      787              
  Lines       46643    46763     +120     
==========================================
+ Hits        10211    10304      +93     
- Misses      35555    35576      +21     
- Partials      877      883       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

carpawell

BTW, not valuable to me as a separate PR. Not clear why we need it, operation params are questionable, the issue does not describe why this should be done (it appeared earlier than the one we are trying to resolve now).

carpawell · 2024-02-27T22:55:31Z

pkg/local_object_storage/blobstor/fstree/fstree.go

+		return nil, fmt.Errorf("stat object file %q: %w", p, err)
+	}
+	sz := fi.Size()
+	if sz > math.MaxInt {


what this check does save us from?
@roman-khimov

Only from garbage in the storage maybe.

otherwise data cannot be fully read

but how int64 can be bigger than MaxInt? do we target such systems? can we run on such systems?

on 32-bit systems

do we target such systems? can we run on such systems?

yes, yes

for me it is no, no. i had to google examples of such machines (meaning years of their production), also it seems that you have a file/object bigger than such machines can have ram

machines (meaning years of their production)

this may be at the OS level

anyway, i dont see such limitation in neither docs nor code

pkg/local_object_storage/blobstor/fstree/fstree.go

carpawell · 2024-02-27T23:23:41Z

pkg/local_object_storage/blobstor/fstree/fstree.go

+		b = make([]byte, sz)
+	}
+
+	_, err = io.ReadFull(f, b)


why not streaming?

u mean return io.Reader? no need for now, may be added later

we have big problems with memory, i thought that extending the storage interface with row data should be done for memory optimizations mainly

for memory optimizations

it is: as i mentioned in the commit, in some cases we wanna skip decode-encode round

i mean reading a full object in memory at once is not good to me in a PR that tries to beat memory problems

PR that tries to beat memory problems

it doesn't. Main commit says what problem it solves - excessive decode-encode round

cthulhu-rider · 2024-02-28T04:44:46Z

Not clear why we need it

tried to explain in the commit. This is 100%-needed functionality for any storage IMO - just get what u store as []byte

roman-khimov

We need better coverage as well.

pkg/local_object_storage/blobstor/common/get.go

roman-khimov · 2024-02-28T20:10:11Z

pkg/local_object_storage/blobstor/fstree/fstree.go

+		return nil, fmt.Errorf("stat object file %q: %w", p, err)
+	}
+	sz := fi.Size()
+	if sz > math.MaxInt {


Only from garbage in the storage maybe.

pkg/local_object_storage/blobstor/fstree/fstree.go

pkg/local_object_storage/blobstor/common/get.go

pkg/local_object_storage/engine/get.go

pkg/local_object_storage/shard/get.go

cthulhu-rider · 2024-03-01T10:49:35Z

increased test coverage. Fixed old bug on the way

roman-khimov · 2024-03-03T10:55:15Z

We need to have some broader context for this. It looks like a beginning of a custom memory allocator which just shouldn't be done. We can mmap things, we can stream things, we can pool some chunks (but it doesn't work well for objects of different sizes unfortunately). If nothing else works in-place allocation is an option too (which we had for years anyway, just in a bit different form), especially given that it will be more effective already (one flat buffer, no decoding).

cthulhu-rider · 2024-03-04T09:11:31Z

ok lets use make only for now

cthulhu-rider · 2024-03-04T12:59:52Z

dropped options

Previously, the local object storage engine always decoded the requested objects. In cases where it was necessary to obtain a binary object, an excessive decoding-encoding round was performed. Now `GetBytes` method to read object binary is provided. Closes #1723. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

Previously, `Shard.Get*` methods returned unset meta presence flag if when object was requested and found in the metabase. The documentation did not explain why. Callers do not isolate the case of a cache hit (this is not visible from the return anyway). In total, this behavior was recognized as incorrect and fixed. Refs 69e1e6c. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

carpawell

Lets try it, but IMO we need object streaming RPC for a correct optimization.

cthulhu-rider · 2024-03-05T08:31:49Z

we need object streaming RPC for a correct optimization

two different topics. Here we cover redundant decoding, while ur talking about smaller bufferization (which is also good to be done, but unrelated to the current PR/issue)

carpawell · 2024-03-06T11:16:05Z

two different topics

Not different to me, the main idea is the same.

ur talking about smaller bufferization

Im talking about less memory consumption and/or allocations number. RPC that streams an object with partial reading may improve important things by orders of magnitude, while reading the whole object into memory and transferring it as a single message should only make it a few times better.

cthulhu-rider requested review from carpawell and roman-khimov as code owners February 26, 2024 15:00

cthulhu-rider force-pushed the 1723-storage-get-binary branch from 0ceb48b to 8654d4f Compare February 26, 2024 15:02

carpawell reviewed Feb 27, 2024

View reviewed changes

roman-khimov requested changes Feb 28, 2024

View reviewed changes

cthulhu-rider force-pushed the 1723-storage-get-binary branch from 8654d4f to 06a5d3b Compare March 1, 2024 10:48

cthulhu-rider force-pushed the 1723-storage-get-binary branch 2 times, most recently from b306b0d to 5123eda Compare March 1, 2024 10:59

cthulhu-rider requested review from carpawell and roman-khimov March 1, 2024 11:02

cthulhu-rider force-pushed the 1723-storage-get-binary branch from 5123eda to cf86ba6 Compare March 4, 2024 12:58

cthulhu-rider added 2 commits March 4, 2024 18:17

cthulhu-rider force-pushed the 1723-storage-get-binary branch from cf86ba6 to 09e385c Compare March 4, 2024 14:18

roman-khimov approved these changes Mar 4, 2024

View reviewed changes

carpawell approved these changes Mar 5, 2024

View reviewed changes

cthulhu-rider merged commit 14b650c into master Mar 5, 2024

cthulhu-rider deleted the 1723-storage-get-binary branch March 5, 2024 08:39

cthulhu-rider mentioned this pull request Apr 5, 2024

engine: Support writing already encoded objects #2801

Merged

Conversation

cthulhu-rider commented Feb 26, 2024

Uh oh!

codecov bot commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

carpawell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carpawell Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cthulhu-rider Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cthulhu-rider commented Feb 28, 2024

Uh oh!

roman-khimov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cthulhu-rider commented Mar 1, 2024

Uh oh!

roman-khimov commented Mar 3, 2024

Uh oh!

cthulhu-rider commented Mar 4, 2024

Uh oh!

cthulhu-rider commented Mar 4, 2024

Uh oh!

carpawell left a comment

Choose a reason for hiding this comment

Uh oh!

cthulhu-rider commented Mar 5, 2024

Uh oh!

carpawell commented Mar 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Feb 26, 2024 •

edited

Loading

carpawell Mar 5, 2024 •

edited

Loading

cthulhu-rider Mar 5, 2024 •

edited

Loading