Skip to content

std.StaticStringMap: bump eval branch quota #19823

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 3, 2024

Conversation

travisstaloch
Copy link
Contributor

@travisstaloch travisstaloch commented May 1, 2024

closes #19803 by changing quota from (30 * N) to (10 * N * log2(N)) where
N = kvs_list.len

  • adds reported adversarial test case
  • update doc comment of getLongestPrefix()

EDITED

@mlugg
Copy link
Member

mlugg commented May 1, 2024

Since the KVs are sorted, a linearly-growing bound will never be sufficient for extreme cases. Perhaps grow proportional to n^2 or something?

@travisstaloch travisstaloch force-pushed the static-str-map-followup branch from 52df5d0 to eb162bd Compare May 1, 2024 00:29
@travisstaloch
Copy link
Contributor Author

Since the KVs are sorted, a linearly-growing bound will never be sufficient for extreme cases. Perhaps grow proportional to n^2 or something?

Thanks, sounds good! Checking if test-std passes after changing quota to (10 * N * N) where N = kvs_list.len.

@travisstaloch travisstaloch force-pushed the static-str-map-followup branch from eb162bd to 8be5c5a Compare May 1, 2024 00:35
@travisstaloch
Copy link
Contributor Author

Since the KVs are sorted, a linearly-growing bound will never be sufficient for extreme cases. Perhaps grow proportional to n^2 or something?

Done.

@travisstaloch travisstaloch force-pushed the static-str-map-followup branch from 8be5c5a to 58321b7 Compare May 1, 2024 00:46
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

N squared??? NOT OK

If StaticStringMap was recently changed to be N^2 then it needs to be reverted pronto

@travisstaloch
Copy link
Contributor Author

If StaticStringMap was recently changed to be N^2 then it needs to be reverted pronto

The comptime initialization has not changed significantly from the original ComptimeStringMap impl. The limit was changed from 1500 to 30*N. But this was found to be insufficient for adversarial key arrangements. The N^2 limit is needed for sorting those keys which may require N^2 steps.

@andrewrk
Copy link
Member

andrewrk commented May 1, 2024

Sorting should be O(N * log2(N)). Related: #13642

@travisstaloch
Copy link
Contributor Author

Sounds good. I will update the limit to C * N * log2(N).

@andrewrk
Copy link
Member

andrewrk commented May 1, 2024

Great. I love that it documents the compilation time complexity.

closes ziglang#19803 by changing quota from (30 * N) to (10 * N * log2(N)) where
N = kvs_list.len

* adds reported adversarial test case
* update doc comment of getLongestPrefix()
@travisstaloch travisstaloch force-pushed the static-str-map-followup branch from 58321b7 to c8795e1 Compare May 1, 2024 04:11
@travisstaloch travisstaloch requested a review from andrewrk May 1, 2024 04:12
@travisstaloch
Copy link
Contributor Author

Just pushed a change. Now using math.log2_int_ceil(usize, kvs_list.len) instead.

@andrewrk andrewrk merged commit 44db92d into ziglang:master May 3, 2024
@andrewrk
Copy link
Member

andrewrk commented May 3, 2024

Thanks!

@travisstaloch travisstaloch deleted the static-str-map-followup branch May 3, 2024 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StaticStringMap evaluation branch quota too low
3 participants