Remove lpAssertValidEntry from listpack functions to achieve up to 10% improvement on `HSET` command.

[ashtul]

This PR come after a similar PR was declined on Redis repo redis/redis#11273 with additional info at redis/redis#11293.

The issue it comes to solve is excess checks for corrupted data whenever a listpack is traversing. There are flamecharts in the original PR show it can reach 10% improvement of common commands such a HSET

The reasoning against the change is that the data can be corrupted if the RESTORE command was used and with flag SANITIZE_DUMP_NO.

IMO this isn't justified. To make all users pay a significant patently because a user can potentially load data without sanitizing it first, seems excessive. Anyone who does it must consider the server may crush.

This simple change can give ValKey a nice speed boost.

NOTE: the current tests fail b/c there are tests which load corrupted listpacks without sanitizing them first.

[madolson]

@zuiderkwast I see you commented on the previous thread, are you aligned with this? This seems like a good idea.

[zuiderkwast]

@madolson Yes, the original PR first just removed the validation and Oran complained that there may be corrupt data from unvalidated RDB or RESTORE. I think it's better to always validate on insert rather than on lookup, even though it may affect RDB loading time.

@ashtul do you want to make a flamegraph of loading a dump with and without sanitizing? It would be good to have a view on this difference as well, just for reference.

[zuiderkwast]

Here are some stats from when deep sanitization were introduced: https://github.com/redis/redis/pull/7807#issuecomment-736718188. RESTORE deep is 80% slower than RESTORE shallow.

[zuiderkwast]

If we always sanitize listpacks on load and no longer on lookup, then we shall also do the same for intset and stream.

For zipmap and ziplist (no longer used) I think we always convert them when we load them from an old dump. Let's check that santitization is done in this case too.

Then we should remove the config and ACL rules added in https://github.com/redis/redis/pull/7807. (We can't remove them immediately but we can make them have no effect.)

@madolson WDYT?

[ashtul]

@madolson @zuiderkwast

The stats Oran showed might be skewed due to other improvements he has done as he writes Note that initially LPOS and HGET showed severe (-25%) degradation, and after some optimizations effort (last commit) i was able to re-gain the performance loss and even improve..

What do you suggest validating an insert? Maybe validating prev, next and the following next should be EOF.

As for stats, I will try to create them after the holiday.

BTW, how hard would it be to add mechanism which will load data without sanitation but won't add the key until it is sanitized, possibly on a thread?

[zuiderkwast]

What do you suggest validating an insert? Maybe validating prev, next and the following next should be EOF.

We only need to validate when we're loading a dump that can contain corrupt data (RESTORE or RDB). A normal insert can't add invalid data.

As for stats, I will try to create them after the holiday.

Sounds good. Anyway, I can accept a little slower RDB loading. It's more important to avoid validate on lookup. Also, the system is simpler if we never load corrupt data.

BTW, how hard would it be to add mechanism which will load data without sanitation but won't add the key until it is sanitized, possibly on a thread?

I think that would be too complex. Are you suggesting using a background thread for that? Like #356? We can do that later maybe, if you have a good idea about it, but not in the same PR.

[daniel-house]

It seems like all of this is the classic trade-off between security and speed. Since we can't have both, and there are situations that justify either choice, it appears to me that we should be giving the solution architect the ability to choose via a new configuration property.

[zuiderkwast]

@daniel-house There is already a config for that. Just the possibility of having invalid data is what prevents us from removing these asserts.

More important than load speed vs lookup speed IMO is the complexity aspect. The possibility of having invalid data in memory is a tech dept with a high maintenance cost IMO.

[zuiderkwast]

@valkey-io/core-team Shall we remove the possibility to load potentially corrupt data? Yes :+1: or no :-1: (This is a core team vote, so non-core-team members, please don't vote on this comment. Feel free to comment in the thread though.)

[daniel-house]

There is already a config for that. Just the possibility of having invalid data is what prevents us from removing these asserts.

It seems I was unclear. I meant to put a config around removing these asserts. Allow the asserts to be disabled via a test performed inside the in-lined functions.

[madolson]

Before I vote, to make sure I understand, the plan is to change the code so we always sanitize the load on RDB Restore and RDB Load so that we don't need to do these checks during runtime. I'm OK with that decision as long as we have performance numbers that we aren't dramatically (>25%) increased execution time on the load.

[madolson]

@ashtul Sorry for the delay. Do you have any interest in updating this PR to perform the validation on the restore path. This would improve the normal runtime performance, and only slightly degrade the restore case which should happen less often.

[madolson]

The decision was approved if we add the validation on the restore path.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 70.03%. Comparing base (04d76d8) to head (f44150e). Report is 30 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #399      +/-   ##
============================================
- Coverage     70.43%   70.03%   -0.40%     
============================================
  Files           113      114       +1     
  Lines         61728    61624     -104     
============================================
- Hits          43479    43160     -319     
- Misses        18249    18464     +215     

Files with missing lines	Coverage Δ
src/listpack.c	83.90% <100.00%> (-6.32%)	:arrow_down:

... and 33 files with indirect coverage changes

[ashtul]

@madolson @zuiderkwast This is great. Removing this unnecessary assert will speed up valkey for many use cases.

I need help with the tests. I am unfamiliar with tcl and my solution of removing whole tests is not likely to be the right solution. Is there is anyone I can get help from? I plan to come to Vienna for the conference, would it be better to discuss it then?

[ashtul]

Hi @zuiderkwast, I have rolled back some changes I have made since I don't have the knowledge in tcl and the attention span to fix the tests correctly. The change in this PR is important and I do not want to drag committing it. I'm pretty sure some users will notice an increase in ops per second and lower latency as soon as we push it into the unstable branch. It actually gives valkey an advantage over redis. Can someone takes the PR from here?

[zuiderkwast]

The change in this PR is important and I do not want to drag committing it.

We are releasing 8.0 very soon. We are busy fixing the stability of this release, so this PR will have to wait until after 8.0 is released.

What is missing in this PR is to make the config sanitize-dump-payload no behave just like sanitize-dump-payload yes, so we always sanitize the payload when we load it.

[ashtul]

@zuiderkwast @madolson With the help of @mnunberg1, the fail tests were removed. CI tests do not run and are awaiting your approval. Is that a bug?