Skip to content

Conversation

mordante
Copy link
Member

In addition to changes in the tables the extended grapheme clustering algorithm has been overhauled. Before I considered a separate state machine to implement the rules. With the new rule GB9c this became more attractive and the design has changed.

This change initially had quite an impact on the performance. By making the state machine persistent the performance was improved greatly. Note it is still slower than before due to the larger Unicode tables.

Before

Benchmark Time CPU Iterations

BM_ascii_text 1891 ns 1889 ns 369504
BM_unicode_text 106642 ns 106397 ns 6576
BM_cyrillic_text 73420 ns 73277 ns 9445
BM_japanese_text 62485 ns 62387 ns 11153
BM_emoji_text 1895 ns 1893 ns 369525
BM_ascii_text<wchar_t> 2015 ns 2013 ns 346887
BM_unicode_text<wchar_t> 92119 ns 92017 ns 7598
BM_cyrillic_text<wchar_t> 62637 ns 62568 ns 11117
BM_japanese_text<wchar_t> 53850 ns 53785 ns 12803
BM_emoji_text<wchar_t> 2016 ns 2014 ns 347325

After

Benchmark Time CPU Iterations

BM_ascii_text 1906 ns 1904 ns 369409
BM_unicode_text 265462 ns 265175 ns 2628
BM_cyrillic_text 181063 ns 180865 ns 3871
BM_japanese_text 130927 ns 130789 ns 5324
BM_emoji_text 1892 ns 1890 ns 370537
BM_ascii_text<wchar_t> 2038 ns 2035 ns 343689
BM_unicode_text<wchar_t> 277603 ns 277282 ns 2526
BM_cyrillic_text<wchar_t> 188558 ns 188339 ns 3727
BM_japanese_text<wchar_t> 133084 ns 132943 ns 5262
BM_emoji_text<wchar_t> 2012 ns 2010 ns 348015

Persistent

Benchmark Time CPU Iterations

BM_ascii_text 1904 ns 1899 ns 367472
BM_unicode_text 133609 ns 133287 ns 5246
BM_cyrillic_text 90185 ns 89941 ns 7796
BM_japanese_text 75137 ns 74946 ns 9316
BM_emoji_text 1906 ns 1901 ns 368081
BM_ascii_text<wchar_t> 2703 ns 2696 ns 259153
BM_unicode_text<wchar_t> 131497 ns 131168 ns 5341
BM_cyrillic_text<wchar_t> 87071 ns 86840 ns 8076
BM_japanese_text<wchar_t> 72279 ns 72099 ns 9682
BM_emoji_text<wchar_t> 2021 ns 2016 ns 346767

@mordante mordante requested a review from a team as a code owner March 25, 2024 17:35
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Mar 25, 2024
@llvmbot
Copy link
Member

llvmbot commented Mar 25, 2024

@llvm/pr-subscribers-libcxx

Author: Mark de Wever (mordante)

Changes

In addition to changes in the tables the extended grapheme clustering algorithm has been overhauled. Before I considered a separate state machine to implement the rules. With the new rule GB9c this became more attractive and the design has changed.

This change initially had quite an impact on the performance. By making the state machine persistent the performance was improved greatly. Note it is still slower than before due to the larger Unicode tables.

Before

Benchmark Time CPU Iterations

BM_ascii_text<char> 1891 ns 1889 ns 369504
BM_unicode_text<char> 106642 ns 106397 ns 6576
BM_cyrillic_text<char> 73420 ns 73277 ns 9445
BM_japanese_text<char> 62485 ns 62387 ns 11153
BM_emoji_text<char> 1895 ns 1893 ns 369525
BM_ascii_text<wchar_t> 2015 ns 2013 ns 346887
BM_unicode_text<wchar_t> 92119 ns 92017 ns 7598
BM_cyrillic_text<wchar_t> 62637 ns 62568 ns 11117
BM_japanese_text<wchar_t> 53850 ns 53785 ns 12803
BM_emoji_text<wchar_t> 2016 ns 2014 ns 347325

After

Benchmark Time CPU Iterations

BM_ascii_text<char> 1906 ns 1904 ns 369409
BM_unicode_text<char> 265462 ns 265175 ns 2628
BM_cyrillic_text<char> 181063 ns 180865 ns 3871
BM_japanese_text<char> 130927 ns 130789 ns 5324
BM_emoji_text<char> 1892 ns 1890 ns 370537
BM_ascii_text<wchar_t> 2038 ns 2035 ns 343689
BM_unicode_text<wchar_t> 277603 ns 277282 ns 2526
BM_cyrillic_text<wchar_t> 188558 ns 188339 ns 3727
BM_japanese_text<wchar_t> 133084 ns 132943 ns 5262
BM_emoji_text<wchar_t> 2012 ns 2010 ns 348015

Persistent

Benchmark Time CPU Iterations

BM_ascii_text<char> 1904 ns 1899 ns 367472
BM_unicode_text<char> 133609 ns 133287 ns 5246
BM_cyrillic_text<char> 90185 ns 89941 ns 7796
BM_japanese_text<char> 75137 ns 74946 ns 9316
BM_emoji_text<char> 1906 ns 1901 ns 368081
BM_ascii_text<wchar_t> 2703 ns 2696 ns 259153
BM_unicode_text<wchar_t> 131497 ns 131168 ns 5341
BM_cyrillic_text<wchar_t> 87071 ns 86840 ns 8076
BM_japanese_text<wchar_t> 72279 ns 72099 ns 9682
BM_emoji_text<wchar_t> 2021 ns 2016 ns 346767


Patch is 784.11 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/86543.diff

19 Files Affected:

  • (modified) libcxx/docs/ReleaseNotes/19.rst (+1)
  • (modified) libcxx/include/CMakeLists.txt (+1)
  • (modified) libcxx/include/__format/escaped_output_table.h (+6-5)
  • (added) libcxx/include/__format/indic_conjunct_break_table.h (+350)
  • (modified) libcxx/include/__format/unicode.h (+209-95)
  • (modified) libcxx/include/__format/width_estimation_table.h (+3-4)
  • (modified) libcxx/include/libcxx.imp (+1)
  • (modified) libcxx/include/module.modulemap (+1)
  • (modified) libcxx/test/libcxx/utilities/format/format.string/format.string.std/extended_grapheme_cluster.h (+1959-204)
  • (modified) libcxx/test/libcxx/utilities/format/format.string/format.string.std/extended_grapheme_cluster.pass.cpp (+15)
  • (modified) libcxx/utils/CMakeLists.txt (+8)
  • (modified) libcxx/utils/data/unicode/DerivedCoreProperties.txt (+267-10)
  • (modified) libcxx/utils/data/unicode/DerivedGeneralCategory.txt (+12-10)
  • (modified) libcxx/utils/data/unicode/EastAsianWidth.txt (+2586-2584)
  • (modified) libcxx/utils/data/unicode/GraphemeBreakProperty.txt (+3-3)
  • (modified) libcxx/utils/data/unicode/GraphemeBreakTest.txt (+656-71)
  • (modified) libcxx/utils/data/unicode/emoji-data.txt (+5-5)
  • (modified) libcxx/utils/generate_extended_grapheme_cluster_table.py (+11-20)
  • (added) libcxx/utils/generate_indic_conjunct_break_table.py (+309)
diff --git a/libcxx/docs/ReleaseNotes/19.rst b/libcxx/docs/ReleaseNotes/19.rst
index dd39c1bbbc78a3..50d5940795a268 100644
--- a/libcxx/docs/ReleaseNotes/19.rst
+++ b/libcxx/docs/ReleaseNotes/19.rst
@@ -53,6 +53,7 @@ Improvements and New Features
   resulting in a performance increase of up to 1400x.
 - The ``std::mismatch`` algorithm has been optimized for integral types, which can lead up to 40x performance
   improvements.
+- The formatting library is updated to Unicode 15.1.0.
 
 Deprecations and Removals
 -------------------------
diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index 982b85e4e2d62f..4990294b6d76b2 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -382,6 +382,7 @@ set(files
   __format/format_error.h
   __format/format_functions.h
   __format/format_parse_context.h
+  __format/indic_conjunct_break_table.h
   __format/format_string.h
   __format/format_to_n_result.h
   __format/formatter.h
diff --git a/libcxx/include/__format/escaped_output_table.h b/libcxx/include/__format/escaped_output_table.h
index e9f4a6e4f63f56..b194f9431c3be3 100644
--- a/libcxx/include/__format/escaped_output_table.h
+++ b/libcxx/include/__format/escaped_output_table.h
@@ -110,7 +110,7 @@ namespace __escaped_output_table {
 /// - bits [0, 10] The size of the range, allowing 2048 elements.
 /// - bits [11, 31] The lower bound code point of the range. The upper bound of
 ///   the range is lower bound + size.
-_LIBCPP_HIDE_FROM_ABI inline constexpr uint32_t __entries[893] = {
+_LIBCPP_HIDE_FROM_ABI inline constexpr uint32_t __entries[894] = {
     0x00000020,
     0x0003f821,
     0x00056800,
@@ -464,14 +464,14 @@ _LIBCPP_HIDE_FROM_ABI inline constexpr uint32_t __entries[893] = {
     0x0174d000,
     0x0177a00b,
     0x017eb019,
-    0x017fe004,
+    0x01800000,
     0x01815005,
     0x01820000,
     0x0184b803,
     0x01880004,
     0x01898000,
     0x018c7800,
-    0x018f200b,
+    0x018f200a,
     0x0190f800,
     0x05246802,
     0x05263808,
@@ -1000,8 +1000,9 @@ _LIBCPP_HIDE_FROM_ABI inline constexpr uint32_t __entries[893] = {
     0x15b9d005,
     0x15c0f001,
     0x1675100d,
-    0x175f0fff,
-    0x179f0c1e,
+    0x175f080e,
+    0x1772f7ff,
+    0x17b2f1a1,
     0x17d0f5e1,
     0x189a5804};
 
diff --git a/libcxx/include/__format/indic_conjunct_break_table.h b/libcxx/include/__format/indic_conjunct_break_table.h
new file mode 100644
index 00000000000000..44521d27498c3c
--- /dev/null
+++ b/libcxx/include/__format/indic_conjunct_break_table.h
@@ -0,0 +1,350 @@
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// WARNING, this entire header is generated by
+// utils/generate_indic_conjunct_break_table.py
+// DO NOT MODIFY!
+
+// UNICODE, INC. LICENSE AGREEMENT - DATA FILES AND SOFTWARE
+//
+// See Terms of Use <https://www.unicode.org/copyright.html>
+// for definitions of Unicode Inc.'s Data Files and Software.
+//
+// NOTICE TO USER: Carefully read the following legal agreement.
+// BY DOWNLOADING, INSTALLING, COPYING OR OTHERWISE USING UNICODE INC.'S
+// DATA FILES ("DATA FILES"), AND/OR SOFTWARE ("SOFTWARE"),
+// YOU UNEQUIVOCALLY ACCEPT, AND AGREE TO BE BOUND BY, ALL OF THE
+// TERMS AND CONDITIONS OF THIS AGREEMENT.
+// IF YOU DO NOT AGREE, DO NOT DOWNLOAD, INSTALL, COPY, DISTRIBUTE OR USE
+// THE DATA FILES OR SOFTWARE.
+//
+// COPYRIGHT AND PERMISSION NOTICE
+//
+// Copyright (c) 1991-2022 Unicode, Inc. All rights reserved.
+// Distributed under the Terms of Use in https://www.unicode.org/copyright.html.
+//
+// Permission is hereby granted, free of charge, to any person obtaining
+// a copy of the Unicode data files and any associated documentation
+// (the "Data Files") or Unicode software and any associated documentation
+// (the "Software") to deal in the Data Files or Software
+// without restriction, including without limitation the rights to use,
+// copy, modify, merge, publish, distribute, and/or sell copies of
+// the Data Files or Software, and to permit persons to whom the Data Files
+// or Software are furnished to do so, provided that either
+// (a) this copyright and permission notice appear with all copies
+// of the Data Files or Software, or
+// (b) this copyright and permission notice appear in associated
+// Documentation.
+//
+// THE DATA FILES AND SOFTWARE ARE PROVIDED "AS IS", WITHOUT WARRANTY OF
+// ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE
+// WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+// NONINFRINGEMENT OF THIRD PARTY RIGHTS.
+// IN NO EVENT SHALL THE COPYRIGHT HOLDER OR HOLDERS INCLUDED IN THIS
+// NOTICE BE LIABLE FOR ANY CLAIM, OR ANY SPECIAL INDIRECT OR CONSEQUENTIAL
+// DAMAGES, OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
+// DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
+// TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THE DATA FILES OR SOFTWARE.
+//
+// Except as contained in this notice, the name of a copyright holder
+// shall not be used in advertising or otherwise to promote the sale,
+// use or other dealings in these Data Files or Software without prior
+// written authorization of the copyright holder.
+
+#ifndef _LIBCPP___FORMAT_INDIC_CONJUNCT_BREAK_TABLE_H
+#define _LIBCPP___FORMAT_INDIC_CONJUNCT_BREAK_TABLE_H
+
+#include <__algorithm/ranges_upper_bound.h>
+#include <__config>
+#include <__iterator/access.h>
+#include <cstddef>
+#include <cstdint>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+#  pragma GCC system_header
+#endif
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+#if _LIBCPP_STD_VER >= 20
+
+namespace __indic_conjunct_break {
+
+enum class __property : uint8_t {
+  // Values generated from the data files.
+  __Consonant,
+  __Extend,
+  __Linker,
+
+  // The code unit has none of above properties.
+  __none
+};
+
+/// The entries of the indic conjunct break property table.
+///
+/// The data is generated from
+/// -  https://www.unicode.org/Public/UCD/latest/ucd/DerivedCoreProperties.txt
+///
+/// The data has 3 values
+/// - bits [0, 1] The property. One of the values generated from the datafiles
+///   of \ref __property
+/// - bits [2, 10] The size of the range.
+/// - bits [11, 31] The lower bound code point of the range. The upper bound of
+///   the range is lower bound + size.
+///
+/// The 9 bits for the size allow a maximum range of 512 elements. Some ranges
+/// in the Unicode tables are larger. They are stored in multiple consecutive
+/// ranges in the data table. An alternative would be to store the sizes in a
+/// separate 16-bit value. The original MSVC STL code had such an approach, but
+/// this approach uses less space for the data and is about 4% faster in the
+/// following benchmark.
+/// libcxx/benchmarks/std_format_spec_string_unicode.bench.cpp
+// clang-format off
+_LIBCPP_HIDE_FROM_ABI inline constexpr uint32_t __entries[201] = {
+    0x00180139,
+    0x001a807d,
+    0x00241811,
+    0x002c88b1,
+    0x002df801,
+    0x002e0805,
+    0x002e2005,
+    0x002e3801,
+    0x00308029,
+    0x00325851,
+    0x00338001,
+    0x0036b019,
+    0x0036f815,
+    0x00373805,
+    0x0037500d,
+    0x00388801,
+    0x00398069,
+    0x003f5821,
+    0x003fe801,
+    0x0040b00d,
+    0x0040d821,
+    0x00412809,
+    0x00414811,
+    0x0042c809,
+    0x0044c01d,
+    0x0046505d,
+    0x00471871,
+    0x0048a890,
+    0x0049e001,
+    0x004a6802,
+    0x004a880d,
+    0x004ac01c,
+    0x004bc01c,
+    0x004ca84c,
+    0x004d5018,
+    0x004d9000,
+    0x004db00c,
+    0x004de001,
+    0x004e6802,
+    0x004ee004,
+    0x004ef800,
+    0x004f8004,
+    0x004ff001,
+    0x0051e001,
+    0x0054a84c,
+    0x00555018,
+    0x00559004,
+    0x0055a810,
+    0x0055e001,
+    0x00566802,
+    0x0057c800,
+    0x0058a84c,
+    0x00595018,
+    0x00599004,
+    0x0059a810,
+    0x0059e001,
+    0x005a6802,
+    0x005ae004,
+    0x005af800,
+    0x005b8800,
+    0x0060a84c,
+    0x0061503c,
+    0x0061e001,
+    0x00626802,
+    0x0062a805,
+    0x0062c008,
+    0x0065e001,
+    0x0068a894,
+    0x0069d805,
+    0x006a6802,
+    0x0071c009,
+    0x0072400d,
+    0x0075c009,
+    0x0076400d,
+    0x0078c005,
+    0x0079a801,
+    0x0079b801,
+    0x0079c801,
+    0x007b8805,
+    0x007ba001,
+    0x007bd00d,
+    0x007c0001,
+    0x007c1009,
+    0x007c3005,
+    0x007e3001,
+    0x0081b801,
+    0x0081c805,
+    0x00846801,
+    0x009ae809,
+    0x00b8a001,
+    0x00be9001,
+    0x00bee801,
+    0x00c54801,
+    0x00c9c809,
+    0x00d0b805,
+    0x00d30001,
+    0x00d3a81d,
+    0x00d3f801,
+    0x00d58035,
+    0x00d5f83d,
+    0x00d9a001,
+    0x00db5821,
+    0x00dd5801,
+    0x00df3001,
+    0x00e1b801,
+    0x00e68009,
+    0x00e6a031,
+    0x00e71019,
+    0x00e76801,
+    0x00e7a001,
+    0x00e7c005,
+    0x00ee00fd,
+    0x01006801,
+    0x01068031,
+    0x01070801,
+    0x0107282d,
+    0x01677809,
+    0x016bf801,
+    0x016f007d,
+    0x01815015,
+    0x0184c805,
+    0x05337801,
+    0x0533a025,
+    0x0534f005,
+    0x05378005,
+    0x05416001,
+    0x05470045,
+    0x05495809,
+    0x054d9801,
+    0x05558001,
+    0x05559009,
+    0x0555b805,
+    0x0555f005,
+    0x05560801,
+    0x0557b001,
+    0x055f6801,
+    0x07d8f001,
+    0x07f1003d,
+    0x080fe801,
+    0x08170001,
+    0x081bb011,
+    0x08506801,
+    0x08507801,
+    0x0851c009,
+    0x0851f801,
+    0x08572805,
+    0x0869200d,
+    0x08755805,
+    0x0877e809,
+    0x087a3029,
+    0x087c100d,
+    0x08838001,
+    0x0883f801,
+    0x0885d001,
+    0x08880009,
+    0x08899805,
+    0x088b9801,
+    0x088e5001,
+    0x0891b001,
+    0x08974805,
+    0x0899d805,
+    0x089b3019,
+    0x089b8011,
+    0x08a23001,
+    0x08a2f001,
+    0x08a61801,
+    0x08ae0001,
+    0x08b5b801,
+    0x08b95801,
+    0x08c1d001,
+    0x08c9f001,
+    0x08ca1801,
+    0x08d1a001,
+    0x08d23801,
+    0x08d4c801,
+    0x08ea1001,
+    0x08ea2005,
+    0x08ecb801,
+    0x08fa1001,
+    0x0b578011,
+    0x0b598019,
+    0x0de4f001,
+    0x0e8b2801,
+    0x0e8b3809,
+    0x0e8b7011,
+    0x0e8bd81d,
+    0x0e8c2819,
+    0x0e8d500d,
+    0x0e921009,
+    0x0f000019,
+    0x0f004041,
+    0x0f00d819,
+    0x0f011805,
+    0x0f013011,
+    0x0f047801,
+    0x0f098019,
+    0x0f157001,
+    0x0f17600d,
+    0x0f27600d,
+    0x0f468019,
+    0x0f4a2019};
+// clang-format on
+
+/// Returns the indic conjuct break property of a code point.
+[[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr __property __get_property(const char32_t __code_point) noexcept {
+  // The algorithm searches for the upper bound of the range and, when found,
+  // steps back one entry. This algorithm is used since the code point can be
+  // anywhere in the range. After a lower bound is found the next step is to
+  // compare whether the code unit is indeed in the range.
+  //
+  // Since the entry contains a code unit, size, and property the code point
+  // being sought needs to be adjusted. Just shifting the code point to the
+  // proper position doesn't work; suppose an entry has property 0, size 1,
+  // and lower bound 3. This results in the entry 0x1810.
+  // When searching for code point 3 it will search for 0x1800, find 0x1810
+  // and moves to the previous entry. Thus the lower bound value will never
+  // be found.
+  // The simple solution is to set the bits belonging to the property and
+  // size. Then the upper bound for code point 3 will return the entry after
+  // 0x1810. After moving to the previous entry the algorithm arrives at the
+  // correct entry.
+  ptrdiff_t __i = std::ranges::upper_bound(__entries, (__code_point << 11) | 0x7ffu) - __entries;
+  if (__i == 0)
+    return __property::__none;
+
+  --__i;
+  uint32_t __upper_bound = (__entries[__i] >> 11) + ((__entries[__i] >> 2) & 0b1'1111'1111);
+  if (__code_point <= __upper_bound)
+    return static_cast<__property>(__entries[__i] & 0b11);
+
+  return __property::__none;
+}
+
+} // namespace __indic_conjunct_break
+
+#endif //_LIBCPP_STD_VER >= 20
+
+_LIBCPP_END_NAMESPACE_STD
+
+#endif // _LIBCPP___FORMAT_INDIC_CONJUNCT_BREAK_TABLE_H
diff --git a/libcxx/include/__format/unicode.h b/libcxx/include/__format/unicode.h
index 40067ca3448bb7..229cda44239c6a 100644
--- a/libcxx/include/__format/unicode.h
+++ b/libcxx/include/__format/unicode.h
@@ -15,6 +15,7 @@
 #include <__concepts/same_as.h>
 #include <__config>
 #include <__format/extended_grapheme_cluster_table.h>
+#include <__format/indic_conjunct_break_table.h>
 #include <__iterator/concepts.h>
 #include <__iterator/readable_traits.h> // iter_value_t
 #include <string_view>
@@ -292,84 +293,223 @@ class __code_point_view<wchar_t> {
 };
 #    endif // _LIBCPP_HAS_NO_WIDE_CHARACTERS
 
-_LIBCPP_HIDE_FROM_ABI constexpr bool __at_extended_grapheme_cluster_break(
-    bool& __ri_break_allowed,
-    bool __has_extened_pictographic,
-    __extended_grapheme_custer_property_boundary::__property __prev,
-    __extended_grapheme_custer_property_boundary::__property __next) {
-  using __extended_grapheme_custer_property_boundary::__property;
+// State machine to implement the Extended Grapheme Cluster Boundary
+//
+// The exact rules may change between Unicode versions.
+// This implements the extended rules see
+// https://www.unicode.org/reports/tr29/#Grapheme_Cluster_Boundaries
+class __extended_grapheme_cluster_break {
+  using __EGC_property  = __extended_grapheme_custer_property_boundary::__property;
+  using __inCB_property = __indic_conjunct_break::__property;
 
-  __has_extened_pictographic |= __prev == __property::__Extended_Pictographic;
+public:
+  _LIBCPP_HIDE_FROM_ABI constexpr explicit __extended_grapheme_cluster_break(char32_t __first_code_point)
+      : __prev_code_point_(__first_code_point),
+        __prev_property_(__extended_grapheme_custer_property_boundary::__get_property(__first_code_point)) {
+    // Initializes the active rule.
+    if (__prev_property_ == __EGC_property::__Extended_Pictographic)
+      __active_rule_ = __rule::__GB11_emoji;
+    else if (__prev_property_ == __EGC_property::__Regional_Indicator)
+      __active_rule_ = __rule::__GB12_GB13_regional_indicator;
+    else if (__indic_conjunct_break::__get_property(__first_code_point) == __inCB_property::__Consonant)
+      __active_rule_ = __rule::__GB9c_indic_conjunct_break;
+  }
 
-  // https://www.unicode.org/reports/tr29/tr29-39.html#Grapheme_Cluster_Boundary_Rules
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr bool operator()(char32_t __next_code_point) {
+    __EGC_property __next_property = __extended_grapheme_custer_property_boundary::__get_property(__next_code_point);
+    bool __result                  = __evaluate(__next_code_point, __next_property);
+    __prev_code_point_             = __next_code_point;
+    __prev_property_               = __next_property;
+    return __result;
+  }
 
-  // *** Break at the start and end of text, unless the text is empty. ***
+  // The code point whose break propery are considered during the next
+  // evaluation cyle.
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr char32_t __current_code_point() const { return __prev_code_point_; }
 
-  _LIBCPP_ASSERT_INTERNAL(__prev != __property::__sot, "should be handled in the constructor"); // GB1
-  _LIBCPP_ASSERT_INTERNAL(__prev != __property::__eot, "should be handled by our caller");      // GB2
+private:
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr bool
+  __evaluate(char32_t __next_code_point, __EGC_property __next_property) {
+    switch (__active_rule_) {
+    case __rule::__none:
+      return __evaluate_none(__next_code_point, __next_property);
+    case __rule::__GB9c_indic_conjunct_break:
+      return __evaluate_GB9c_indic_conjunct_break(__next_code_point, __next_property);
+    case __rule::__GB11_emoji:
+      return __evaluate_GB11_emoji(__next_code_point, __next_property);
+    case __rule::__GB12_GB13_regional_indicator:
+      return __evaluate_GB12_GB13_regional_indicator(__next_code_point, __next_property);
+    }
+  }
 
-  // *** Do not break between a CR and LF. Otherwise, break before and after controls. ***
-  if (__prev == __property::__CR && __next == __property::__LF) // GB3
-    return false;
+  _LIBCPP_HIDE_FROM_ABI constexpr bool __evaluate_none(char32_t __next_code_point, __EGC_property __next_property) {
+    // *** Break at the start and end of text, unless the text is empty. ***
 
-  if (__prev == __property::__Control || __prev == __property::__CR || __prev == __property::__LF) // GB4
-    return true;
+    _LIBCPP_ASSERT_INTERNAL(__prev_property_ != __EGC_property::__sot, "should be handled in the constructor"); // GB1
+    _LIBCPP_ASSERT_INTERNAL(__prev_property_ != __EGC_property::__eot, "should be handled by our caller");      // GB2
 
-  if (__next == __property::__Control || __next == __property::__CR || __next == __property::__LF) // GB5
-    return true;
+    // *** Do not break between a CR and LF. Otherwise, break before and after controls. ***
+    if (__prev_property_ == __EGC_property::__CR && __next_property == __EGC_property::__LF) // GB3
+      return false;
 
-  // *** Do not break Hangul syllable sequences. ***
-  if (__prev == __property::__L && (__next == __property::__L || __next == __property::__V ||
-                                    __next == __property::__LV || __next == __property::__LVT)) // GB6
-    return false;
+    if (__prev_property_ == __EGC_property::__Control || __prev_property_ == __EGC_property::__CR ||
+        __prev_property_ == __EGC_property::__LF) // GB4
+      return true;
 
-  if ((__prev == __property::__LV || __prev == __property::__V) &&
-      (__next == __property::__V || __next == __property::__T)) // GB7
-    return false;
+    if (__next_property == __EGC_property::__Control || __next_property == __EGC_property::__CR ||
+        __next_property == __EGC_property::__LF) // GB5
+      return true;
 
-  if ((__prev == __property::__LVT || __prev == __property::__T) && __next == __property::__T) // GB8
-    return false;
+    // *** Do not break Hangul syllable sequences. ***
+    if (__prev_property_ == __EGC_property::__L &&
+        (__next_property == __EGC_property::__L || __next_property == __EGC_property::__V ||
+         __next_property == __EGC_property::__LV || __next_property == __EGC_property::__LVT)) // GB6
+      return false;
 
-  // *** Do not break before extending characters or ZWJ. ***
-  if (__next == __property::__Extend || __next == __property::__ZWJ)
-    return false; // GB9
+    if ((__prev_property_ == __EGC_property::__LV || __prev_property_ == __EGC_property::__V) &&
+        (__next_property == __EGC_property::__V || __next_property == __EGC_property::__T)) // GB7
+      return false;
 
-  // *** Do not break before SpacingMarks, or after Prepend characters. ***
-  if (__next == __property::__SpacingMark) // GB9a
-    return false;
+    if ((__prev_property_ == __EGC_property::__LVT || __prev_property_ == __EGC_property::__T) &&
+        __next_property == __EGC_property::__T) // GB8
+      return false;
 
-  if (__prev == __property::__Prepend) // GB9b
-    return false;
+    // *** Do not break before extending characters or ZWJ. ***
+    if (__next_property == __EGC_property::__Extend || __next_property == __EGC_property::__ZWJ)
+      return false; // GB9
 
-  // *** Do not break within emoji modifier sequences or emoji zwj sequences. ***
+    // *** Do not break before SpacingMarks, or after Prepend characters. ***
+    if (__next_property == __EGC_property::__SpacingMark) // GB9a
+      return false;
 
-  // GB11 \p{Extended_Pictographic} Extend* ZWJ x \p{Extended_Pictographic}
-  //
-  // Note that several parts of this rule are matched by GB9: Any x (Extend | ZWJ)
-  // - \p{Extended_Pictographic} x Extend
-  // - Extend x Extend
-  // - \p{Extended_Pictographic} x ZWJ
-  // - Extend x ZWJ
-  //
-  // So the only case left to test is
-  // - \p{Extended_Pictographic}' x ZWJ x \p{Extended_Pictographic}
-  //   where  \p{Extended_Pictographic}' is stored in __has_extened_pictographic
-  if (__has_extened_pictographic && __prev == __property::__ZWJ && __next == __property::__E...
[truncated]

Copy link

github-actions bot commented Mar 25, 2024

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff d179176f3e3505044ad14be19ccb3459a70206a9 00cb8e6ae813184118408412763439ca350a4fff -- libcxx/include/__format/indic_conjunct_break_table.h libcxx/include/__format/escaped_output_table.h libcxx/include/__format/unicode.h libcxx/include/__format/width_estimation_table.h libcxx/test/libcxx/utilities/format/format.string/format.string.std/extended_grapheme_cluster.h libcxx/test/libcxx/utilities/format/format.string/format.string.std/extended_grapheme_cluster.pass.cpp
View the diff from clang-format here.
diff --git a/libcxx/test/libcxx/utilities/format/format.string/format.string.std/extended_grapheme_cluster.h b/libcxx/test/libcxx/utilities/format/format.string/format.string.std/extended_grapheme_cluster.h
index eb7500a828..799baf45bc 100644
--- a/libcxx/test/libcxx/utilities/format/format.string/format.string.std/extended_grapheme_cluster.h
+++ b/libcxx/test/libcxx/utilities/format/format.string/format.string.std/extended_grapheme_cluster.h
@@ -82,8 +82,8 @@ struct data {
 };
 
 /// The data for UTF-8.
-std::array<data<char>, 1187> data_utf8 = {{
-     {"\U00000020\U00000020", {32, 32}, {1, 2}},
+std::array<data<char>, 1187> data_utf8 = {
+    {{"\U00000020\U00000020", {32, 32}, {1, 2}},
      {"\U00000020\U00000308\U00000020", {32, 32}, {3, 4}},
      {"\U00000020\U0000000d", {32, 13}, {1, 2}},
      {"\U00000020\U00000308\U0000000d", {32, 13}, {3, 4}},
@@ -1277,8 +1277,8 @@ std::array<data<char>, 1187> data_utf8 = {{
 /// since the size of the code units differ the breaks can contain different
 /// values.
 #ifndef TEST_HAS_NO_WIDE_CHARACTERS
-std::array<data<wchar_t>, 1187> data_utf16 = {{
-     {L"\U00000020\U00000020", {32, 32}, {1, 2}},
+std::array<data<wchar_t>, 1187> data_utf16 = {
+    {{L"\U00000020\U00000020", {32, 32}, {1, 2}},
      {L"\U00000020\U00000308\U00000020", {32, 32}, {2, 3}},
      {L"\U00000020\U0000000d", {32, 13}, {1, 2}},
      {L"\U00000020\U00000308\U0000000d", {32, 13}, {2, 3}},
@@ -2471,8 +2471,8 @@ std::array<data<wchar_t>, 1187> data_utf16 = {{
 /// Note that most of the data for the UTF-16 and UTF-32 are identical. However
 /// since the size of the code units differ the breaks can contain different
 /// values.
-std::array<data<wchar_t>, 1187> data_utf32 = {{
-     {L"\U00000020\U00000020", {32, 32}, {1, 2}},
+std::array<data<wchar_t>, 1187> data_utf32 = {
+    {{L"\U00000020\U00000020", {32, 32}, {1, 2}},
      {L"\U00000020\U00000308\U00000020", {32, 32}, {2, 3}},
      {L"\U00000020\U0000000d", {32, 13}, {1, 2}},
      {L"\U00000020\U00000308\U0000000d", {32, 13}, {2, 3}},

Copy link

github-actions bot commented Mar 25, 2024

⚠️ Python code formatter, darker found issues in your code. ⚠️

You can test this locally with the following command:
darker --check --diff -r d179176f3e3505044ad14be19ccb3459a70206a9...00cb8e6ae813184118408412763439ca350a4fff libcxx/utils/generate_indic_conjunct_break_table.py libcxx/utils/generate_extended_grapheme_cluster_table.py
View the diff from darker here.
--- generate_indic_conjunct_break_table.py	2024-03-25 19:12:44.000000 +0000
+++ generate_indic_conjunct_break_table.py	2024-03-25 19:15:30.039238 +0000
@@ -36,10 +36,11 @@
 
 
 LINE_REGEX = re.compile(
     r"^(?P<lower>[0-9A-F]{4,5})(?:\.\.(?P<upper>[0-9A-F]{4,5}))?\s*;\s*InCB;\s*(?P<prop>\w+)"
 )
+
 
 def parsePropertyLine(inputLine: str) -> Optional[PropertyRange]:
     result = PropertyRange()
     if m := LINE_REGEX.match(inputLine):
         lower_str, upper_str, result.prop = m.group("lower", "upper", "prop")
@@ -49,11 +50,10 @@
             result.upper = int(upper_str, base=16)
         return result
 
     else:
         return None
-
 
 
 def compactPropertyRanges(input: list[PropertyRange]) -> list[PropertyRange]:
     """
     Merges consecutive ranges with the same property to one range.
@@ -293,11 +293,13 @@
     with derived_core_path.open(encoding="utf-8") as f:
         indic_conjunct_break_ranges = compactPropertyRanges(
             [x for line in f if (x := parsePropertyLine(line))]
         )
 
-    indic_conjunct_break_data = generate_cpp_data("Grapheme_Break", indic_conjunct_break_ranges)
+    indic_conjunct_break_data = generate_cpp_data(
+        "Grapheme_Break", indic_conjunct_break_ranges
+    )
     return "\n".join([indic_conjunct_break_data])
 
 
 if __name__ == "__main__":
     if len(sys.argv) == 2:

@mordante mordante force-pushed the review/unicode_15.1 branch 3 times, most recently from bc6ef86 to 00cb8e6 Compare March 25, 2024 19:13
Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

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

Mostly LGTM with a few questions / comments.

libcxx-generate-extended-grapheme-cluster-tests
libcxx-generate-escaped-output-table
libcxx-generate-width-estimation-table
libcxx-indic-conjunct-break-table
Copy link
Member

Choose a reason for hiding this comment

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

Weird indentation. Tabs?

__extended_grapheme_custer_property_boundary::__property __prev,
__extended_grapheme_custer_property_boundary::__property __next) {
using __extended_grapheme_custer_property_boundary::__property;
// State machine to implement the Extended Grapheme Cluster Boundary
Copy link
Member

Choose a reason for hiding this comment

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

Do we not need any updated tests for these changes to the algorithm?

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 we do need updates, and I did. See my comment below.

__extended_grapheme_custer_property_boundary::__property __prev,
__extended_grapheme_custer_property_boundary::__property __next) {
using __extended_grapheme_custer_property_boundary::__property;
// State machine to implement the Extended Grapheme Cluster Boundary
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 we do need updates, and I did. See my comment below.


static_assert(count_entries(inCB::__property::__Linker) == 6);
static_assert(count_entries(inCB::__property::__Consonant) == 240);
static_assert(count_entries(inCB::__property::__Extend) == 884);
Copy link
Member Author

Choose a reason for hiding this comment

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

@ldionne These are the tests that we properly parse the new properties.

The data tables for this test are in "extended_grapheme_cluster.h". This file is generated from the Extended Grapheme Cluster break test provided by Unicode in the file "GraphemeBreakTest.txt". I use this test to validate whether the state machine is correct. This test has been updated in Unicode 15.1.0 to test the new GB9c rule.

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

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

LGTM, I had misunderstood how this gets tested. Thanks!

In addition to changes in the tables the extended grapheme clustering
algorithm has been overhauled. Before I considered a separate state
machine to implement the rules. With the new rule GB9c this became more
attractive and the design has changed.

This change initially had quite an impact on the performance. By
making the state machine persistent the performance was improved
greatly. Note it is still slower than before due to the larger Unicode
tables.

Before
--------------------------------------------------------------------
Benchmark                          Time             CPU   Iterations
--------------------------------------------------------------------
BM_ascii_text<char>             1891 ns         1889 ns       369504
BM_unicode_text<char>         106642 ns       106397 ns         6576
BM_cyrillic_text<char>         73420 ns        73277 ns         9445
BM_japanese_text<char>         62485 ns        62387 ns        11153
BM_emoji_text<char>             1895 ns         1893 ns       369525
BM_ascii_text<wchar_t>          2015 ns         2013 ns       346887
BM_unicode_text<wchar_t>       92119 ns        92017 ns         7598
BM_cyrillic_text<wchar_t>      62637 ns        62568 ns        11117
BM_japanese_text<wchar_t>      53850 ns        53785 ns        12803
BM_emoji_text<wchar_t>          2016 ns         2014 ns       347325

After
--------------------------------------------------------------------
Benchmark                          Time             CPU   Iterations
--------------------------------------------------------------------
BM_ascii_text<char>             1906 ns         1904 ns       369409
BM_unicode_text<char>         265462 ns       265175 ns         2628
BM_cyrillic_text<char>        181063 ns       180865 ns         3871
BM_japanese_text<char>        130927 ns       130789 ns         5324
BM_emoji_text<char>             1892 ns         1890 ns       370537
BM_ascii_text<wchar_t>          2038 ns         2035 ns       343689
BM_unicode_text<wchar_t>      277603 ns       277282 ns         2526
BM_cyrillic_text<wchar_t>     188558 ns       188339 ns         3727
BM_japanese_text<wchar_t>     133084 ns       132943 ns         5262
BM_emoji_text<wchar_t>          2012 ns         2010 ns       348015

Persistent
--------------------------------------------------------------------
Benchmark                          Time             CPU   Iterations
--------------------------------------------------------------------
BM_ascii_text<char>             1904 ns         1899 ns       367472
BM_unicode_text<char>         133609 ns       133287 ns         5246
BM_cyrillic_text<char>         90185 ns        89941 ns         7796
BM_japanese_text<char>         75137 ns        74946 ns         9316
BM_emoji_text<char>             1906 ns         1901 ns       368081
BM_ascii_text<wchar_t>          2703 ns         2696 ns       259153
BM_unicode_text<wchar_t>      131497 ns       131168 ns         5341
BM_cyrillic_text<wchar_t>      87071 ns        86840 ns         8076
BM_japanese_text<wchar_t>      72279 ns        72099 ns         9682
BM_emoji_text<wchar_t>          2021 ns         2016 ns       346767
@mordante mordante force-pushed the review/unicode_15.1 branch from 00cb8e6 to c0fb71c Compare April 9, 2024 17:19
@mordante mordante merged commit 59e66c5 into llvm:main Apr 9, 2024
@mordante mordante deleted the review/unicode_15.1 branch April 9, 2024 17:20
mordante pushed a commit that referenced this pull request Jul 22, 2024
…99621)

- [P2160R1][] "Locks lock lockables"
- [P2212R2][] "Relax Requirements for `time_point::clock`"
- [P1675R2][] "`rethrow_exception` must be allowed to copy"
- [P2340R1][] "Clarifying the status of the 'C headers'"
- [P2460R2][] "Relax requirements on `wchar_t` to match existing
practices"

Are all papers that change wording without changing implementation
behaviour.

Additionally, [P2736R2][] "Referencing The Unicode Standard", is an
already complete paper in 19.0 (as of [LLVM-86543][])

[P2160R1]: https://wg21.link/p2160r1
[P2212R2]: https://wg21.link/p2212r2
[P1675R2]: https://wg21.link/p1675r2
[P2340R1]: https://wg21.link/p2340r1
[P2460R2]: https://wg21.link/p2460r2
[P2736R2]: https://wg21.link/p2736r2
[LLVM-86543]: #86543
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
…99621)

Summary:
- [P2160R1][] "Locks lock lockables"
- [P2212R2][] "Relax Requirements for `time_point::clock`"
- [P1675R2][] "`rethrow_exception` must be allowed to copy"
- [P2340R1][] "Clarifying the status of the 'C headers'"
- [P2460R2][] "Relax requirements on `wchar_t` to match existing
practices"

Are all papers that change wording without changing implementation
behaviour.

Additionally, [P2736R2][] "Referencing The Unicode Standard", is an
already complete paper in 19.0 (as of [LLVM-86543][])

[P2160R1]: https://wg21.link/p2160r1
[P2212R2]: https://wg21.link/p2212r2
[P1675R2]: https://wg21.link/p1675r2
[P2340R1]: https://wg21.link/p2340r1
[P2460R2]: https://wg21.link/p2460r2
[P2736R2]: https://wg21.link/p2736r2
[LLVM-86543]: #86543

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251122
blueboxd pushed a commit to blueboxd/libcxx that referenced this pull request Apr 19, 2025
…(#99621)

- [P2160R1][] "Locks lock lockables"
- [P2212R2][] "Relax Requirements for `time_point::clock`"
- [P1675R2][] "`rethrow_exception` must be allowed to copy"
- [P2340R1][] "Clarifying the status of the 'C headers'"
- [P2460R2][] "Relax requirements on `wchar_t` to match existing
practices"

Are all papers that change wording without changing implementation
behaviour.

Additionally, [P2736R2][] "Referencing The Unicode Standard", is an
already complete paper in 19.0 (as of [LLVM-86543][])

[P2160R1]: https://wg21.link/p2160r1
[P2212R2]: https://wg21.link/p2212r2
[P1675R2]: https://wg21.link/p1675r2
[P2340R1]: https://wg21.link/p2340r1
[P2460R2]: https://wg21.link/p2460r2
[P2736R2]: https://wg21.link/p2736r2
[LLVM-86543]: llvm/llvm-project#86543

NOKEYCHECK=True
GitOrigin-RevId: 418922623673a8044d30a57825b026a03f9d9354
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants