Deleting variables with long numeric-only list indices causes severe lag spikes

[REIKO-KR]

Skript/Server Version

Server Version: 1.21.11-168-ea288d3 (MC: 1.21.11)
Skript Version: 2.15.3 (skriptlang-github)
Installed Skript Addons:
 - skript-placeholders v1.7.1 (https://github.com/APickledWalrus/skript-placeholders)
 - DiSky v4.29.0
 - skript-reflect v2.6.3 (https://github.com/SkriptLang/skript-reflect)
 - skript-db v1.5.0
 - SkBee v3.18.3 (https://github.com/ShaneBeee/SkBee)
 - SkRedis v2.3.2
Installed dependencies:
 - Vault v1.7.3-b131
 - WorldGuard v7.0.17+2370-e42d8bc

Bug Description

Deleting a variable whose list index is a long numeric-only value can cause a noticeable server lag spike.

I noticed this while using long numeric IDs as list variable indices, such as Discord snowflake IDs or timestamp-like IDs. Creating or setting the variable is fine, but deleting it causes the server to freeze briefly.

Example problematic index:

853420195002187776

When the same value is prefixed with a non-numeric character, such as:

id_853420195002187776

the lag spike does not happen.

Expected Behavior

Deleting a variable should not cause a large lag spike depending on whether the list index is numeric-only.

These two cases should have similar performance:

delete {test::853420195002187776}
delete {test::id_853420195002187776}

Steps to Reproduce

1. Load this test script:

command /testnumericdelete:
	trigger:
		set {_id} to "853420195002187776"
		set {delete-test::%{_id}%} to "test"

		set {_start} to unix timestamp of now
		send "Before numeric-only delete"
		delete {delete-test::%{_id}%}
		set {_elapsed} to (unix timestamp of now) - {_start}

		send "After numeric-only delete"
		send "numeric-only delete elapsed: %{_elapsed}% seconds"

command /testprefixeddelete:
	trigger:
		set {_id} to "id_853420195002187776"
		set {delete-test::%{_id}%} to "test"

		set {_start} to unix timestamp of now
		send "Before prefixed delete"
		delete {delete-test::%{_id}%}
		set {_elapsed} to (unix timestamp of now) - {_start}

		send "After prefixed delete"
		send "prefixed delete elapsed: %{_elapsed}% seconds"

2. Run /testnumericdelete.
3. Check the elapsed time message and whether the server briefly freezes.
4. Run /testprefixeddelete.
5. Compare the elapsed time and server behavior.

In my production script, changing mailbox IDs from numeric-only timestamp keys to prefixed keys fixed the lag immediately:

# Lagged when deleting
{mailbox::%uuid%::1780000000000000000}

# No lag when deleting
{mailbox::%uuid%::m_1780000000000000000}

Errors or Screenshots

Other

No response

Agreement

• I have read the guidelines above and affirm I am following them with this report.

[skriptlang-automation]

🔍 Triage Checklist

Type

• bug
  • Confirm you can reproduce before continuing
• suggestion
  • Apply relevant classification labels (scripting, syntax, docs, etc.)

Priority

• priority: lowest
• priority: low
• priority: medium
• priority: high
• priority: critical

Project

• 2.15 Releases
• 2.16 Releases

---

• ✅ Mark as triaged

[sovdeeth]

will test later but likely due to recomputeMaxIndex. I've seen it seemingly trigger large garbage collections for some reason. May be based on how the underlying set functions?

[bluelhf]

It's a hash set so the contains() should be trivial, but a large index is a large index. This can be fixed by keeping an ordered set of the indices, but it's a memory-time tradeoff

[REIKO-KR]

I think this specific case might be caused by an integer overflow in parsePositiveInt.

I agree that recomputeMaxIndex() is probably where the expensive loop happens, but I think the very large index may not be a valid index in this case. It seems like it could be an overflowed/incorrect value produced while parsing the variable name.

For example, when setting a variable like this:

853420195002187776

handleInsert can be called with an overflowed/incorrect index value:

handleInsert("853420195002187776", 1329594368, ...)

That incorrect value is then stored as maxIndex and added to numericalIndices.

Later, when the same variable is deleted, handleRemove parses the same overflowed value again:

handleRemove("853420195002187776", ...)

After this runs:

numericalIndices.remove(1329594368)

the set becomes empty.

Since index == maxIndex, recomputeMaxIndex() is called:

private void recomputeMaxIndex() {
    while (maxIndex >= 0 && !numericalIndices.contains(maxIndex))
        maxIndex--;
}

At that point, because numericalIndices is empty, contains(maxIndex) keeps returning false, so maxIndex is decremented all the way from 1329594368 down to -1.

So I think the issue is not that HashSet.contains() itself is expensive, but that recomputeMaxIndex() can end up doing around 1.33 billion iterations because maxIndex was set to an overflowed value.

Interestingly, a variable name like this does not seem to trigger the issue:

id_853420195002187776

That seems to be because parsePositiveInt returns -1 as soon as it encounters _.

Then handleInsert / handleRemove exits early due to this check:

if (index < 0) return;

So the overflowed value never gets stored as maxIndex.

One possible fix would be to change the parsing logic from this:

value = Math.addExact(value * 10, c - '0');

to this:

value = Math.addExact(Math.multiplyExact(value, 10), c - '0');

Currently, Math.addExact only catches overflow from the addition, but value * 10 can already overflow before addExact is called.

Using Math.multiplyExact should make multiplication overflow throw an ArithmeticException as well, allowing the existing catch block to return -1.

That would cause handleInsert / handleRemove to exit early and prevent the overflowed value from being stored as maxIndex in the first place.

With this approach, the same numeric-looking variable name is no longer tracked as a numeric index, so the expensive recomputeMaxIndex() loop does not seem to be triggered:

Another possible approach, as mentioned above, would be to keep the numeric parsing behavior unchanged and make recomputeMaxIndex() avoid the linear decrement loop.

For example, numericalIndices could be changed from a HashSet to a TreeSet, so the current maximum index can be obtained directly:

// current
private final Set<Integer> numericalIndices = new HashSet<>();

private void recomputeMaxIndex() {
    while (maxIndex >= 0 && !numericalIndices.contains(maxIndex))
        maxIndex--;
}

// alternative
private final TreeSet<Integer> numericalIndices = new TreeSet<>();

private void recomputeMaxIndex() {
    maxIndex = numericalIndices.isEmpty() ? -1 : numericalIndices.last();
}

With this approach, the overflowed value may still be treated as a numeric index, but recomputeMaxIndex() no longer needs to decrement from that large value one by one:

This would avoid the huge loop even if parsePositiveInt still returns the overflowed value. It also preserves the existing behavior where such values are still treated as numeric indices.

So I think there may be two possible fixes here:

• Check multiplication overflow in parsePositiveInt
  This fixes the specific case at the source by preventing overflowed values from being tracked as numeric indices.

• Use an ordered set such as TreeSet
  This preserves the current parsing behavior, but avoids the expensive recomputation loop.

The TreeSet approach would be a bit more invasive and changes contains() from average O(1) to O(log n), so there is a small memory/time tradeoff.
The Math.multiplyExact approach is smaller, but it changes how very large numeric-looking variable names are classified.

Does this sound plausible?

[bluelhf]

Math.multiplyExact is definitely something we should use here, since judging from the try-catch it was @UnderscoreTud's intention that only indices up to the integer limit are handled.

However, the problem remains that indices up to but not including 2³¹ are slow to delete. Consider this version of the test script:

command /testnumericdelete:
	trigger:
		set {_id} to "%max integer value%"
		set {delete-test::%{_id}%} to "test"

		set {_start} to unix timestamp of now
		send "Before numeric-only delete"
		delete {delete-test::%{_id}%}
		set {_elapsed} to (unix timestamp of now) - {_start}

		send "After numeric-only delete"
		send "numeric-only delete elapsed: %{_elapsed}% seconds"

command /testprefixeddelete:
	trigger:
		set {_id} to "id_%max integer value%"
		set {delete-test::%{_id}%} to "test"

		set {_start} to unix timestamp of now
		send "Before prefixed delete"
		delete {delete-test::%{_id}%}
		set {_elapsed} to (unix timestamp of now) - {_start}

		send "After prefixed delete"
		send "prefixed delete elapsed: %{_elapsed}% seconds"

This gives performance results even worse than the initial test, even with Math.multiplyExact:

As for using TreeSet to track the indices, this fixes the issue cleanly but care should be taken to ensure the performance of adding elements doesn't degrade. I wonder if we can use higher() to iterate the set more efficiently when it is sparse. As always remember Rob Pike's rules one and two; use a profiler!!!

[sovdeeth]

good analysis thanks you two! I believe tud originally tried a TreeSet when working on the og pr and ran into performance degredation. Worth trying again though since it does seem like the best solution of the obvious fixes

[bluelhf]

good analysis thanks you two! I believe tud originally tried a TreeSet when working on the og pr and ran into performance degredation. Worth trying again though since it does seem like the best solution of the obvious fixes

Some ideas based purely on theory:

• You can make the TreeSet's next index scan faster by keeping a binary tree over the full universe [0, 2³¹[ and pruning filled nodes from the search
• A run-length encoded bitmap would have good space complexity and very fast next index scan but poor update performance
• A j.u.l.BitSet would be very fast tpp but always consume 256 MiB of RAM
• A Y-fast trie would work otherwise but I'm not sure how you'd find the next index, I guess by repeated successors starting at 0
• TreeSet<Gap> would provide insertion, next vacancy, deletion, and membership test logarithmic w.r.t. the number of gaps and would only degrade in performance for indices like (1, 3, 5, 7, 9). It is good in theory but likely slower than what we have now for 95 % of users
• We can also just ignore the problem, mostly nobody wants indices this high to actually be treated as numeric, so they can simply add a prefix to the index to make them non-numeric

[sovdeeth]

good analysis thanks you two! I believe tud originally tried a TreeSet when working on the og pr and ran into performance degredation. Worth trying again though since it does seem like the best solution of the obvious fixes

Some ideas based purely on theory:

* You can make the TreeSet's next index scan faster by keeping a binary tree over the full universe [0, 2³¹[ and pruning filled nodes from the search

* A run-length encoded bitmap would have good space complexity and very fast next index scan but poor update performance

* A `j.u.l.BitSet` would be very fast tpp but always consume 256 MiB of RAM

* A Y-fast trie would work otherwise but I'm not sure how you'd find the next index, I guess by repeated successors starting at 0

* `TreeSet<Gap>` would provide insertion, next vacancy, deletion, and membership test logarithmic w.r.t. the number of gaps and would only degrade in performance for indices like (1, 3, 5, 7, 9). It is good in theory but likely slower than what we have now for 95 % of users

* We can also just ignore the problem, mostly nobody wants indices this high to _actually_ be treated as numeric, so they can simply add a prefix to the index to make them non-numeric

yeah we could even just say "ignore numeric indices over 10,000" and 99% of issues here are likely to vanish.