Skip to content

Case insensitivity for tags#24

Open
ddssff wants to merge 6 commits intobgamari:masterfrom
seereason:ci
Open

Case insensitivity for tags#24
ddssff wants to merge 6 commits intobgamari:masterfrom
seereason:ci

Conversation

@ddssff
Copy link
Copy Markdown
Contributor

@ddssff ddssff commented Jun 17, 2023

If I've bench-marked correctly this appears to reduce performance by about 3%.

@bgamari
Copy link
Copy Markdown
Owner

bgamari commented Jun 19, 2023

It's quite neat how small this change is. Good work!

@ddssff
Copy link
Copy Markdown
Contributor Author

ddssff commented Jun 19, 2023

Oops, I think -DCASE_INSENSITIVE should be -DCASE_INSENSITIVE=1

@ddssff
Copy link
Copy Markdown
Contributor Author

ddssff commented Jun 19, 2023

I think the performance hit may be more like 28%. With case insensitive patch:

benchmarking Forced/html-parser
time                 10.52 ms   (10.52 ms .. 10.53 ms)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 10.53 ms   (10.53 ms .. 10.54 ms)
std dev              11.79 μs   (10.11 μs .. 14.39 μs)

Without case insensitive patch:

benchmarking Forced/html-parser
time                 8.180 ms   (8.174 ms .. 8.186 ms)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 8.182 ms   (8.179 ms .. 8.192 ms)
std dev              14.49 μs   (6.378 μs .. 26.80 μs)

@bgamari
Copy link
Copy Markdown
Owner

bgamari commented Jun 20, 2023

It looks like there are still testsuite issues, sadly.

@bgamari
Copy link
Copy Markdown
Owner

bgamari commented Jun 20, 2023

@ddssff It would be worth looking at the Core to see why the overhead here is so high.

* Add case-insensitive dependency to benchmark
@ddssff
Copy link
Copy Markdown
Contributor Author

ddssff commented Jun 20, 2023

The slowdown is 100% due to the use of foldCase when building the tags, which comes down to a call to Data.Text.toCaseFold. If I substitute unsafeMk for mk it is as fast as ever. I suppose one way forward might be to make TagName a type parameter of Token and have instances for Text and CI Text. But that is a substantial change.

@ddssff
Copy link
Copy Markdown
Contributor Author

ddssff commented Jun 20, 2023

Not sure what you mean by "Core"

@ddssff
Copy link
Copy Markdown
Contributor Author

ddssff commented Jun 25, 2023

@bgamari the test suite issues are fixed now.

Clifford Beshers added 2 commits April 22, 2026 11:07
…age and hackage.nix kept choosing that version"

This reverts commit b381ee3.
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.

2 participants