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

Bring back the JSPs #1018

Merged
merged 4 commits into from
Jan 29, 2025
Merged

Bring back the JSPs #1018

merged 4 commits into from
Jan 29, 2025

Conversation

eggrobin
Copy link
Member

Four commits, deployed in three builds of the JSPs:

  1. Drop properties.jsp (we should bring something like it back eventually, but besides its abysmal performance it had not been seriously maintained and was becoming borderline useless);
  2. Remove links to UnicodeSet queries for multivalued properties from character.jsp (these queries are very slow as currently implemented—minutes for some of the worst offenders—, and the link was to an invalid query when the character being inspected actually had multiple values anyway);
    • Remove the ICU-based toLowercase etc. pseudoproperties (these are from before the JSPs showed the real Lowercase_Mapping etc. properties, and UnicodeSet queries to compute the preimage of a function are intrinsically very slow);
    • Fail multivalued Unihan property queries if they are in the exact format we used to link to, to deal with residual traffic from links we served before deployment 2., with a suggestion to use a different format in case a hapless human comes across this (this one can hopefully be reverted eventually).

image
image

@eggrobin
Copy link
Member Author

residual traffic from links we served before deployment 2.

This is strangely still a thing, see, e.g., for kJapaneseKun over the past 24 hours (in 15 minute buckets):
image

I guess some crawlers saved those links somewhere…

@@ -145,48 +145,6 @@ public String transform(Integer source) {
},
false)
.setMain("toNFKD", "toNFKD", UnicodeProperty.STRING, "1.1"));

Copy link
Member

Choose a reason for hiding this comment

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

I find these very useful in practice. Is there a compelling reason to remove?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, they are very slow (getSet involves computing the preimage of a function).

But what do you use these for, as opposed to Lowercase_Mapping, Uppercase_Mapping, and Titlecase_Mapping?

Copy link
Member Author

@eggrobin eggrobin Jan 29, 2025

Choose a reason for hiding this comment

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

I just checked (on the staging JSPs which do not yet have these changes), and both of the following contain only characters whose mappings changed in 16.0 (because we are lagging on the ICU version used in the right-hand side):

  • [:^Lowercase_Mapping=@toLowercase@:]
  • [:^Uppercase_Mapping=@toUppercase@:]

[:^Titlecase_Mapping=@toTitlecase@:] contains one interesting difference, U+0345, because ICU skips past a leading U+0345 in titlecasing. But I think for all practical purposes, you can just use the mappings.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, makes sense.

@@ -20,7 +20,8 @@ th { text-align: left }
UtfParameters utfParameters = new UtfParameters(queryString);

String propForValues = utfParameters.getParameter("a");
UnicodeJsp.showPropsTable(out, propForValues, "properties.jsp");
// TODO(egg): Cache this page.
Copy link
Member

Choose a reason for hiding this comment

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

Ok for now. I think just a static page with an explicit list of the properties (and if enumerated, the values) without links would be very helpful for the users, and not hard to generate at build time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. And we should probably grab some information from IndexUnicodeProperties as well, e.g., which files they come from.

But not today.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed

@eggrobin eggrobin merged commit 4381875 into unicode-org:main Jan 29, 2025
15 of 16 checks passed
@macchiati
Copy link
Member

BTW, I tried converting

  • a UnicodeMap to a construct that used TreeMap<Integer, X> + internally generated a Map<X,UnicodeSet> once built.
  • a UnicodeRelation to a construct that used TreeMultimap<Integer, X> in building + internally generated a Map<X,UnicodeSet> once built.

At the expense of additional memory, it sped up the queries significantly.

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