Skip to content
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

address bad usage of TST #4048

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

address bad usage of TST #4048

wants to merge 6 commits into from

Conversation

ywave620
Copy link

@ywave620 ywave620 commented Feb 7, 2025

This relates to performance of parseHeaders

Rationale

Inserting keys to TST in order yields the worst performance of this data structure. It is similar to inserting keys to a binary search tree in order makes it degenerate to a link list.

Randomize the insertion order simply works, but we do not want any potential instable performance, so I take a snapshot of one randomized insertion order that works well

WeChatWorkScreenshot_ea6ff026-5e0d-44b5-8dce-4da5858291c9

Changes

Randomize the insertion order of the well known header keys

Features

N/A

Bug Fixes

N/A

Breaking Changes and Deprecations

N/A

Status

@ywave620 ywave620 changed the title addres bad usage of TST address bad usage of TST Feb 7, 2025
lib/core/tree.js Outdated
94, 86, 15, 9, 56, 25, 38, 49, 12, 73, 24, 60,
46, 11, 14, 83, 40, 52, 13, 21, 48, 30, 45, 10,
53, 5, 59, 44, 35, 16, 36, 32, 0, 51, 82
]
Copy link
Member

Choose a reason for hiding this comment

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

Can you move it to a separate file, or can it be easily generated by a script?

lib/core/tree.js Outdated
// wellknownHeaderNames.forEach((key, idx) => {
// indexes.push(idx)
// })
// randomIndexOfWellknownHeaderNames = shuffleArray(indexes)
Copy link
Member

Choose a reason for hiding this comment

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

why is this commented out? Can you nmove this to a script or another js file instead?

Copy link
Author

Choose a reason for hiding this comment

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

I think we do not want any potential instable performance, right? (even the chance is low :) ) so I take a snapshot of one randomized insertion order that works well

Copy link
Author

Choose a reason for hiding this comment

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

Maybe I just remove them from codebase outright and put it the PR

lib/core/tree.js Outdated Show resolved Hide resolved
@ywave620
Copy link
Author

@metcoder95 could you help move this forward

@tsctx
Copy link
Member

tsctx commented Feb 12, 2025

If not properly shuffled to balance the left and right sides, it will cause a bad case and further degrade performance. This is similar to the problem with binary trees.

@ywave620
Copy link
Author

ywave620 commented Feb 14, 2025

If not properly shuffled to balance the left and right sides, it will cause a bad case and further degrade performance. This is similar to the problem with binary trees.

Given the array is sorted, randomly retrieving item from it and inserting to a TST/Binary search tree should yield a balanced tree. And the bottom line is that we definitely don't want to insert items to a TST in order because this is the worst case of this data structure.

Maybe I can print out the TST and do a comparison? I did a non formal one before and it turned out that when searching for Via we actually saved a lot of node traversing

@metcoder95
Copy link
Member

Seems CI is not happy, can you try to rebase with main and adjust?

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.

4 participants