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]

Now I looked at the diff. It still just removes validation.

If we remove validation on lookup, we must do validation on insert instead.

[zuiderkwast]

Here are some stats from when deep sanitization were introduced: redis/redis#7807 (comment). 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 redis/redis#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 👍 or no 👎 (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.