From a5012e97c4a392d2788777580e0d08b44e036750 Mon Sep 17 00:00:00 2001 From: ariza Date: Mon, 24 Feb 2020 17:09:48 -0800 Subject: [PATCH 1/4] optimize hb_set_del_range() fix issue #2193 --- src/hb-set.hh | 56 ++++++++++++++++++++++++++++++++++++++++++--- test/api/test-set.c | 29 +++++++++++++++++++++++ 2 files changed, 82 insertions(+), 3 deletions(-) diff --git a/src/hb-set.hh b/src/hb-set.hh index 048f09933..822111d59 100644 --- a/src/hb-set.hh +++ b/src/hb-set.hh @@ -89,6 +89,23 @@ struct hb_set_t } } + void del_range (hb_codepoint_t a, hb_codepoint_t b) + { + elt_t *la = &elt (a); + elt_t *lb = &elt (b); + if (la == lb) + *la &= ~((mask (b) << 1) - mask(a)); + else + { + *la &= mask (a) - 1; + la++; + + memset (la, 0, (char *) lb - (char *) la); + + *lb &= ~((mask (b) << 1) - 1); + } + } + bool is_equal (const page_t *other) const { return 0 == hb_memcmp (&v, &other->v, sizeof (v)); @@ -366,14 +383,47 @@ struct hb_set_t dirty (); page->del (g); } + void del_range (hb_codepoint_t a, hb_codepoint_t b) { /* TODO perform op even if !successful. */ - /* TODO Optimize, like add_range(). */ if (unlikely (!successful)) return; - for (unsigned int i = a; i < b + 1; i++) - del (i); + if (unlikely (a > b || a == INVALID || b == INVALID)) return; + dirty (); + unsigned int ma = get_major (a); + unsigned int mb = get_major (b); + unsigned int mds = (a == major_start (ma))? ma: (ma + 1); + int mde = (b + 1 == major_start (mb + 1))? (int)mb: ((int)mb - 1); + if (ma < mds) + { + page_t *page = page_for (a); + if (page) + { + if (ma == mb) + page->del_range (a, b); + else + page->del_range (a, major_start (ma + 1) - 1); + } + } + if (mde < (int)mb && ma != mb) + { + page_t *page = page_for (b); + if (page) + page->del_range (major_start (mb), b); + } + if ((int)mds <= mde) + { + unsigned int write_index = 0; + for (unsigned int i = 0; i < page_map.length; i++) + { + unsigned int m = page_map[i].major; + if (m < mds || mde < (int)m) + page_map[write_index++] = page_map[i]; + } + compact (write_index); + } } + bool get (hb_codepoint_t g) const { const page_t *page = page_for (g); diff --git a/test/api/test-set.c b/test/api/test-set.c index d4e49f753..d80ee8b7d 100644 --- a/test/api/test-set.c +++ b/test/api/test-set.c @@ -470,6 +470,34 @@ test_set_empty (void) hb_set_destroy (b); } +static void +test_set_delrange (void) +{ + hb_set_t *s = hb_set_create (); + + test_empty (s); + for (unsigned int g = 0; g < 2100; g += 10) + hb_set_add (s, g); + + hb_set_add (s, 2047); /* (=512*4-1) edge case */ + + hb_set_del_range (s, 55, 705); + hb_set_del_range (s, 795, 2047); + + g_assert ( hb_set_has (s, 50)); + g_assert (!hb_set_has (s, 60)); + g_assert (!hb_set_has (s, 600)); + g_assert ( hb_set_has (s, 710)); + g_assert ( hb_set_has (s, 790)); + g_assert (!hb_set_has (s, 800)); + g_assert (!hb_set_has (s, 1500)); + g_assert (!hb_set_has (s, 2040)); + g_assert (!hb_set_has (s, 2047)); + g_assert ( hb_set_has (s, 2050)); + + hb_set_destroy (s); +} + int main (int argc, char **argv) { @@ -479,6 +507,7 @@ main (int argc, char **argv) hb_test_add (test_set_algebra); hb_test_add (test_set_iter); hb_test_add (test_set_empty); + hb_test_add (test_set_delrange); hb_test_add (test_set_intersect_empty); hb_test_add (test_set_intersect_page_reduction); From 173b745da8bfd8bda710c90ab48427364068eeb5 Mon Sep 17 00:00:00 2001 From: ariza Date: Mon, 24 Feb 2020 22:56:57 -0800 Subject: [PATCH 2/4] fixed bug & added tests --- src/hb-set.hh | 3 ++- test/api/test-set.c | 10 ++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/hb-set.hh b/src/hb-set.hh index 822111d59..3f5255c03 100644 --- a/src/hb-set.hh +++ b/src/hb-set.hh @@ -392,9 +392,10 @@ struct hb_set_t dirty (); unsigned int ma = get_major (a); unsigned int mb = get_major (b); + /* Delete entire pages from mds through mde. */ unsigned int mds = (a == major_start (ma))? ma: (ma + 1); int mde = (b + 1 == major_start (mb + 1))? (int)mb: ((int)mb - 1); - if (ma < mds) + if ((int)mds > mde || ma < mds) { page_t *page = page_for (a); if (page) diff --git a/test/api/test-set.c b/test/api/test-set.c index d80ee8b7d..b56e64955 100644 --- a/test/api/test-set.c +++ b/test/api/test-set.c @@ -479,14 +479,16 @@ test_set_delrange (void) for (unsigned int g = 0; g < 2100; g += 10) hb_set_add (s, g); + hb_set_add (s, 512); /* edge case */ hb_set_add (s, 2047); /* (=512*4-1) edge case */ - hb_set_del_range (s, 55, 705); + hb_set_del_range (s, 512, 705); hb_set_del_range (s, 795, 2047); - g_assert ( hb_set_has (s, 50)); - g_assert (!hb_set_has (s, 60)); - g_assert (!hb_set_has (s, 600)); + g_assert ( hb_set_has (s, 0)); + g_assert ( hb_set_has (s, 510)); + g_assert (!hb_set_has (s, 512)); + g_assert (!hb_set_has (s, 700)); g_assert ( hb_set_has (s, 710)); g_assert ( hb_set_has (s, 790)); g_assert (!hb_set_has (s, 800)); From de896278f7534c876d28d9b5cf344c5d707d3490 Mon Sep 17 00:00:00 2001 From: ariza Date: Tue, 25 Feb 2020 07:12:20 -0800 Subject: [PATCH 3/4] coding & comment tweaks --- src/hb-set.hh | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/hb-set.hh b/src/hb-set.hh index 3f5255c03..3d5fa6c3f 100644 --- a/src/hb-set.hh +++ b/src/hb-set.hh @@ -392,10 +392,10 @@ struct hb_set_t dirty (); unsigned int ma = get_major (a); unsigned int mb = get_major (b); - /* Delete entire pages from mds through mde. */ - unsigned int mds = (a == major_start (ma))? ma: (ma + 1); - int mde = (b + 1 == major_start (mb + 1))? (int)mb: ((int)mb - 1); - if ((int)mds > mde || ma < mds) + /* Delete pages from ds through de if ds <= de. */ + int ds = (a == major_start (ma))? (int)ma: (int)(ma + 1); + int de = (b + 1 == major_start (mb + 1))? (int)mb: ((int)mb - 1); + if (ds > de || (int)ma < ds) { page_t *page = page_for (a); if (page) @@ -406,19 +406,19 @@ struct hb_set_t page->del_range (a, major_start (ma + 1) - 1); } } - if (mde < (int)mb && ma != mb) + if (de < (int)mb && ma != mb) { page_t *page = page_for (b); if (page) page->del_range (major_start (mb), b); } - if ((int)mds <= mde) + if (ds <= de) { unsigned int write_index = 0; for (unsigned int i = 0; i < page_map.length; i++) { unsigned int m = page_map[i].major; - if (m < mds || mde < (int)m) + if ((int)m < ds || de < (int)m) page_map[write_index++] = page_map[i]; } compact (write_index); From 4081439d2a49f5dfde2f9043b0e53f2008ff211f Mon Sep 17 00:00:00 2001 From: ariza Date: Tue, 25 Feb 2020 15:03:12 -0800 Subject: [PATCH 4/4] tweak reflecting review & add test cases --- src/hb-set.hh | 37 ++++++++++++++++++++-------------- test/api/test-set.c | 49 ++++++++++++++++++++++++++++++--------------- 2 files changed, 55 insertions(+), 31 deletions(-) diff --git a/src/hb-set.hh b/src/hb-set.hh index 3d5fa6c3f..71de7a82c 100644 --- a/src/hb-set.hh +++ b/src/hb-set.hh @@ -384,6 +384,23 @@ struct hb_set_t page->del (g); } + private: + void del_pages (int ds, int de) + { + if (ds <= de) + { + unsigned int write_index = 0; + for (unsigned int i = 0; i < page_map.length; i++) + { + int m = (int) page_map[i].major; + if (m < ds || de < m) + page_map[write_index++] = page_map[i]; + } + compact (write_index); + } + } + + public: void del_range (hb_codepoint_t a, hb_codepoint_t b) { /* TODO perform op even if !successful. */ @@ -393,9 +410,9 @@ struct hb_set_t unsigned int ma = get_major (a); unsigned int mb = get_major (b); /* Delete pages from ds through de if ds <= de. */ - int ds = (a == major_start (ma))? (int)ma: (int)(ma + 1); - int de = (b + 1 == major_start (mb + 1))? (int)mb: ((int)mb - 1); - if (ds > de || (int)ma < ds) + int ds = (a == major_start (ma))? (int) ma: (int) (ma + 1); + int de = (b + 1 == major_start (mb + 1))? (int) mb: ((int) mb - 1); + if (ds > de || (int) ma < ds) { page_t *page = page_for (a); if (page) @@ -406,23 +423,13 @@ struct hb_set_t page->del_range (a, major_start (ma + 1) - 1); } } - if (de < (int)mb && ma != mb) + if (de < (int) mb && ma != mb) { page_t *page = page_for (b); if (page) page->del_range (major_start (mb), b); } - if (ds <= de) - { - unsigned int write_index = 0; - for (unsigned int i = 0; i < page_map.length; i++) - { - unsigned int m = page_map[i].major; - if ((int)m < ds || de < (int)m) - page_map[write_index++] = page_map[i]; - } - compact (write_index); - } + del_pages (ds, de); } bool get (hb_codepoint_t g) const diff --git a/test/api/test-set.c b/test/api/test-set.c index b56e64955..eb690b895 100644 --- a/test/api/test-set.c +++ b/test/api/test-set.c @@ -473,29 +473,46 @@ test_set_empty (void) static void test_set_delrange (void) { + const unsigned P = 512; /* Page size. */ + struct { unsigned b, e; } ranges[] = { + { 35, P-15 }, /* From page middle thru middle. */ + { P, P+100 }, /* From page start thru middle. */ + { P+300, P*2-1 }, /* From page middle thru end. */ + { P*3, P*4+100 }, /* From page start thru next page middle. */ + { P*4+300, P*6-1 }, /* From page middle thru next page end. */ + { P*6+200,P*8+100 }, /* From page middle covering one page thru page middle. */ + { P*9, P*10+105 }, /* From page start covering one page thru page middle. */ + { P*10+305, P*12-1 }, /* From page middle covering one page thru page end. */ + { P*13, P*15-1 }, /* From page start covering two pages thru page end. */ + { P*15+100, P*18+100 } /* From page middle covering two pages thru page middle. */ + }; + unsigned n = sizeof (ranges) / sizeof(ranges[0]); + hb_set_t *s = hb_set_create (); test_empty (s); - for (unsigned int g = 0; g < 2100; g += 10) + for (unsigned int g = 0; g < ranges[n - 1].e + P; g += 2) hb_set_add (s, g); - hb_set_add (s, 512); /* edge case */ - hb_set_add (s, 2047); /* (=512*4-1) edge case */ + hb_set_add (s, P*2-1); + hb_set_add (s, P*6-1); + hb_set_add (s, P*12-1); + hb_set_add (s, P*15-1); - hb_set_del_range (s, 512, 705); - hb_set_del_range (s, 795, 2047); + for (unsigned i = 0; i < n; i++) + hb_set_del_range (s, ranges[i].b, ranges[i].e); + + hb_set_del_range (s, P*13+5, P*15-10); /* Deletion from deleted pages. */ - g_assert ( hb_set_has (s, 0)); - g_assert ( hb_set_has (s, 510)); - g_assert (!hb_set_has (s, 512)); - g_assert (!hb_set_has (s, 700)); - g_assert ( hb_set_has (s, 710)); - g_assert ( hb_set_has (s, 790)); - g_assert (!hb_set_has (s, 800)); - g_assert (!hb_set_has (s, 1500)); - g_assert (!hb_set_has (s, 2040)); - g_assert (!hb_set_has (s, 2047)); - g_assert ( hb_set_has (s, 2050)); + for (unsigned i = 0; i < n; i++) + { + unsigned b = ranges[i].b; + unsigned e = ranges[i].e; + g_assert (hb_set_has (s, (b-2)&~1)); + while (b <= e) + g_assert (!hb_set_has (s, b++)); + g_assert (hb_set_has (s, (e+2)&~1)); + } hb_set_destroy (s); }