Nice! I'm not qualified to approve this code PR, but it sounds like good stuff

The only change is the recent push is dissecting the "Also, this change further narrows the RocksDB iterator's bound." part into a dedicated commit.

The new revision dropped 65b78be.

with this PR the time it took to list 50M objects is shorter

time (nice numactl -N 1 -m 1 -- ~/go/bin/hsbench -a b2345678901234567890 -s b234567890123456789012345678901234567890 -u http://127.0.0.1:8000 -z 4K -d -1 -t $(( $(numactl -N 0 -- nproc) / 1 )) -b 1 -n 50000000 -m l -bp b01b- -op 'folder01/stage01_')

# Before PR: 47:20.61 total
2024/10/09 08:06:42 Running Loop 0 BUCKET LIST TEST
2024/10/09 08:54:02 Loop: 0, Int: TOTAL, Dur(s): 2840.6, Mode: LIST, Ops: 50000, MB/s: 0.00, IO/s: 18, Lat(ms): [ min: 43.5, avg: 56.8, 99%: 76.6, max: 120.5 ], Slowdowns: 0               
( nice numactl -N 1 -m 1 -- ~/go/bin/hsbench -a b2345678901234567890 -s  -u  )  2163.04s user 85.44s system 79% cpu 47:20.61 total

# After PR: 46:58.62 total
2024/10/09 12:26:55 Running Loop 0 BUCKET LIST TEST 
2024/10/09 13:13:53 Loop: 0, Int: TOTAL, Dur(s): 2818.6, Mode: LIST, Ops: 50000, MB/s: 0.00, IO/s: 18, Lat(ms): [ min: 43.0, avg: 56.4, 99%: 76.3, max: 127.1 ], Slowdowns: 0 
( nice numactl -N 1 -m 1 -- ~/go/bin/hsbench -a b2345678901234567890 -s  -u  )  2160.19s user 84.21s system 79% cpu 46:58.62 total

Any idea what the standard deviation is? If I did my math right we're looking at around a 0.8% improvement in this test?

Any idea what the standard deviation is? If I did my math right we're looking at around a 0.8% improvement in this test?

yes, regrettably from repeated tests seems like it is within the noise variability range

@athanatos: @rzarzynski

1. both PRs change the interface. 
   This one does that by extension to free from reasoning about other callers. 
   It makes it more complex but less intrusive. The driving goal was ability to (back)port to many 
   releases without repeating the human-based inspection – only pin-pointed users are affected, 
   interface providers are verified by a compiler (as happened with the reef-main forthporting).
   Whatever the interface becomes, I believe it should be consistent across all implementations of `ObjectStore`.

a) This is simply NOT true. #60056 does not change interface.
One could argue that it does modify a contract, if we had a contract that the iterator after creation seeks to first omap key in object. But its not stated anywhere.
We only have implementation.
And in all use cases we do seek_to_first/lower_bound/upper_bound.
b) Both #60056 and #60000 can be backported. #60056 is smaller.
c) If a backportability without inspection is a goal, one can replace
ceph_assert(seeked);
with
if (!seeked) seek_to_first();
This gives complete equivalence of previous and new behaviour.
d) Our omap implementations in different ObjectStores are already different.
OmapIterators in BlueStore freeze dataset on iterator creation. No seeking will let you see key inserted later.
OmapIterators in KStore and MemStore point to a live database. Changes to db are visible via iterator created earlier.
I do not think we want to invest in unifying the difference.

As a side note, I just realized that my omap bench test uses the ability to create iterator to lock the dataset, and perform seeking later. #60315

2. Seek-at-create allows to squeeze relocking the `Collection::lock`. 
   This doesn't make a difference when listing with extensive iteration (e.g. `max_keys=1000`)
   but the targeted bottleneck is the _list-to-check-for-existence_ case (`max_keys=2`).

True. I am not sure what are the Collection::locks for, but we are taking them.

Ultimately, I think that out path forward should be to get rid of OmapIterator altogether, like proposed here:
https://github.com/ceph/ceph/pull/60278/files#diff-14111846d7c70ada2d719669d9ed93e7d153c198ef1aa93c44b94b00125f6d24R791

@aclamk:

a) This is simply NOT true. #60056 does not change interface.

I disagree. The interface is broader than what can be expressed with language constructs; it's established also by documentation and, particularly in lack of thereof, by implementations. If it had stayed intact, audit of all callers wouldn't have been necessary.

All providers of the OMAP iterator interface do seek-at-create.

b) Both #60056 and #60000 can be backported. #60056 is smaller.

I agree they can be. The question is about risk.

c) If a backportability without inspection is a goal, one can replace
ceph_assert(seeked);
with
if (!seeked) seek_to_first();

Then it would have been far less intrusive.

@markhpc, @mkogan1:

1. hsbench exercises the plain bucket listing instead of the check-for-existence (list with max-keys=2 + prefix). In hsbench.go (from @markhpc's repo):

func runBucketList(thread_num int, stats *Stats) {
        // ...
                // ...
                err := svc.ListObjectsPages(
                        &s3.ListObjectsInput{
                                Bucket:  &buckets[bucket_num],
                                MaxKeys: &max_keys,
                        },

Please note the absence of the prefix S3 parameter.

max_keys is user-configurable with default being 1000:

func init() {
        // ...
        myflag.Int64Var(&max_keys, "mk", 1000, "Maximum number of keys to retreive at once for bucket listings")

The default doesn't cause RGW to paginate the output, so neither start_after will be used.

2. The PR optimizes start_after, filter_prefix and particularly the check-for-existence while doing very little for the plain listing (without the prefix and start_after parameters).
3. To verify the impact on the check-for-existence I extended rados bench to cover OMAP reads (PR #60277).
4. This new & unreviewed benchmark shows 20% of gain for the list-for-existence and roughly the same numbers for the generic, parameterless list.

However, more interesting is profiling under the new workload. In addition to the list-for-existence, it allowed to optimize the plain listing case as well. #60278 is continuation of this PR, also in the matter of backportability being the driving goal.

@rzarzynski Is there any chance I could ask you to submit a PR for hsbench as well that would let us test for this?

Some results from alternative approach:
#60056 (comment)

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

Closing as the far broader #60278 has been merged.