{"thread":{"id":"59180","subject":"[PATCH] delta-islands: free island_marks and bitmaps","startedAt":"2023-02-02T01:03:56Z","lastAt":"2023-02-04T11:15:00Z","messageCount":6,"participants":["Eric Wong","Ævar Arnfjörð Bjarmason","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"471299","messageId":"20230202010353.23391-1-e@80x24.org","threadId":"59180","inReplyTo":null,"subject":"[PATCH] delta-islands: free island_marks and bitmaps","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2023-02-02T01:03:53Z","receivedAt":"2023-02-02T01:03:56Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"On my mirror of linux.git forkgroup with 780 islands, this saves\nnearly 4G of heap memory in pack-objects.  This savings only\nbenefits delta island users of pack bitmaps, as the process\nwould otherwise be exiting anyways.\n\nHowever, there's probably not many delta island users, but the\nmajority of delta island users would also be pack bitmaps users.\n\nSigned-off-by: Eric Wong <e@80x24.org>\n---\n Slowly chipping away, this is low-hanging fruit.\n\n delta-islands.c | 20 +++++++++++++++++++-\n 1 file changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/delta-islands.c b/delta-islands.c\nindex 90c0d6958f..7052a3a6ba 100644\n--- a/delta-islands.c\n+++ b/delta-islands.c\n@@ -513,12 +513,29 @@ void propagate_island_marks(struct commit *commit)\n \t}\n }\n \n+static void free_island_marks(void)\n+{\n+\tstruct island_bitmap *bitmap;\n+\n+\tkh_foreach_value(island_marks, bitmap, {\n+\t\tif (--bitmap->refcount == 0)\n+\t\t\tfree(bitmap);\n+\t});\n+\tkh_destroy_oid_map(island_marks);\n+\tisland_marks = (void *)1; /* crash on unintended future use */\n+}\n+\n int compute_pack_layers(struct packing_data *to_pack)\n {\n \tuint32_t i;\n \n-\tif (!core_island_name || !island_marks)\n+\tif (!island_marks)\n+\t\treturn 1;\n+\n+\tif (!core_island_name) {\n+\t\tfree_island_marks();\n \t\treturn 1;\n+\t}\n \n \tfor (i = 0; i < to_pack->nr_objects; ++i) {\n \t\tstruct object_entry *entry = &to_pack->objects[i];\n@@ -533,6 +550,7 @@ int compute_pack_layers(struct packing_data *to_pack)\n \t\t\t\toe_set_layer(to_pack, entry, 0);\n \t\t}\n \t}\n+\tfree_island_marks();\n \n \treturn 2;\n }\n"},{"id":"471301","messageId":"230202.86mt5wq1i7.gmgdl@evledraar.gmail.com","threadId":"59180","inReplyTo":"20230202010353.23391-1-e@80x24.org","subject":"Re: [PATCH] delta-islands: free island_marks and bitmaps","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-02T01:45:30Z","receivedAt":"2023-02-02T01:50:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Feb 02 2023, Eric Wong wrote:\n\n> +\tkh_foreach_value(island_marks, bitmap, {\n> +\t\tif (--bitmap->refcount == 0)\n\nStyle nit: if (!--x) rather than if (--x == 0) ?\n\n> +\t\t\tfree(bitmap);\n> +\t});\n> +\tkh_destroy_oid_map(island_marks);\n> +\tisland_marks = (void *)1; /* crash on unintended future use */\n\nThis seems counter-productive. If you just leave it free'd then various\nanalysis tools will spot a use-after-free earlier, won't they?\n"},{"id":"471317","messageId":"20230202094217.M955476@dcvr","threadId":"59180","inReplyTo":"230202.86mt5wq1i7.gmgdl@evledraar.gmail.com","subject":"[PATCH v2] delta-islands: free island_marks and bitmaps","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2023-02-02T09:42:17Z","receivedAt":"2023-02-02T09:42:39Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n> On Thu, Feb 02 2023, Eric Wong wrote:\n> \n> > +\tkh_foreach_value(island_marks, bitmap, {\n> > +\t\tif (--bitmap->refcount == 0)\n> \n> Style nit: if (!--x) rather than if (--x == 0) ?\n\nOops, fixed in v2\n\n> > +\t\t\tfree(bitmap);\n> > +\t});\n> > +\tkh_destroy_oid_map(island_marks);\n> > +\tisland_marks = (void *)1; /* crash on unintended future use */\n> \n> This seems counter-productive. If you just leave it free'd then various\n> analysis tools will spot a use-after-free earlier, won't they?\n\n*shrug*  I thought it might be better to use an explicitly bad\npointer to catch lifetime issues that I might've missed from\nreading the code.  Since I've run and tested at this point,\nit probably doesn't matter, now.  So I'll just omit the\nassignment and save some icache footprint.\n\nThanks for the review!\n\n--------8<-------\nSubject: [PATCH] delta-islands: free island_marks and bitmaps\n\nOn my mirror of linux.git forkgroup with 780 islands, this saves\nnearly 4G of heap memory in pack-objects.  This savings only\nbenefits delta island users of pack bitmaps, as the process\nwould otherwise be exiting anyways.\n\nHowever, there's probably not many delta island users, but the\nmajority of delta island users would also be pack bitmaps users.\n\nSigned-off-by: Eric Wong <e@80x24.org>\n---\n delta-islands.c | 19 ++++++++++++++++++-\n 1 file changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/delta-islands.c b/delta-islands.c\nindex 90c0d6958f..c09dab31a4 100644\n--- a/delta-islands.c\n+++ b/delta-islands.c\n@@ -513,12 +513,28 @@ void propagate_island_marks(struct commit *commit)\n \t}\n }\n \n+static void free_island_marks(void)\n+{\n+\tstruct island_bitmap *bitmap;\n+\n+\tkh_foreach_value(island_marks, bitmap, {\n+\t\tif (!--bitmap->refcount)\n+\t\t\tfree(bitmap);\n+\t});\n+\tkh_destroy_oid_map(island_marks);\n+}\n+\n int compute_pack_layers(struct packing_data *to_pack)\n {\n \tuint32_t i;\n \n-\tif (!core_island_name || !island_marks)\n+\tif (!island_marks)\n+\t\treturn 1;\n+\n+\tif (!core_island_name) {\n+\t\tfree_island_marks();\n \t\treturn 1;\n+\t}\n \n \tfor (i = 0; i < to_pack->nr_objects; ++i) {\n \t\tstruct object_entry *entry = &to_pack->objects[i];\n@@ -533,6 +549,7 @@ int compute_pack_layers(struct packing_data *to_pack)\n \t\t\t\toe_set_layer(to_pack, entry, 0);\n \t\t}\n \t}\n+\tfree_island_marks();\n \n \treturn 2;\n }\n"},{"id":"471453","messageId":"Y91OZHrMfca6tb/2@coredump.intra.peff.net","threadId":"59180","inReplyTo":"20230202094217.M955476@dcvr","subject":"Re: [PATCH v2] delta-islands: free island_marks and bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-03T18:11:48Z","receivedAt":"2023-02-03T18:12:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 02, 2023 at 09:42:17AM +0000, Eric Wong wrote:\n\n> > > +\t\t\tfree(bitmap);\n> > > +\t});\n> > > +\tkh_destroy_oid_map(island_marks);\n> > > +\tisland_marks = (void *)1; /* crash on unintended future use */\n> > \n> > This seems counter-productive. If you just leave it free'd then various\n> > analysis tools will spot a use-after-free earlier, won't they?\n> \n> *shrug*  I thought it might be better to use an explicitly bad\n> pointer to catch lifetime issues that I might've missed from\n> reading the code.  Since I've run and tested at this point,\n> it probably doesn't matter, now.  So I'll just omit the\n> assignment and save some icache footprint.\n\nI think it is still worth protecting ourselves. You've run and tested\nthe current code, but the risk remains that somebody will later change\nthe code and introduce a regression.\n\nYes, analysis tools may spot it. But only if we actually trigger the\ncase in question via the test suite, and while running those tools. We\ncan get more consistent behavior by overwriting the pointer.\n\nThe usual thing, of course, would just be to set it to NULL. I guess you\nused \"(void *)1\" in the original because code like in_same_island()\nchecks for NULL. And so it becomes indistinguishable whether we intended\nnot to use islands, or if there's a bug in the program.\n\nIf we are going to reduce the lifetime of island_marks, I think it would\nmake things cleaner to push the \"do we expect to have islands\" boolean\ninto its own variable, something like:\n\ndiff --git a/delta-islands.c b/delta-islands.c\nindex c09dab31a4..7dd4e1419a 100644\n--- a/delta-islands.c\n+++ b/delta-islands.c\n@@ -22,6 +22,7 @@\n \n KHASH_INIT(str, const char *, void *, 1, kh_str_hash_func, kh_str_hash_equal)\n \n+static int using_island_marks;\n static kh_oid_map_t *island_marks;\n static unsigned island_counter;\n static unsigned island_counter_core;\n@@ -96,7 +97,7 @@ int in_same_island(const struct object_id *trg_oid, const struct object_id *src_\n \tkhiter_t trg_pos, src_pos;\n \n \t/* If we aren't using islands, assume everything goes together. */\n-\tif (!island_marks)\n+\tif (!using_island_marks)\n \t\treturn 1;\n \n \t/*\n@@ -124,7 +125,7 @@ int island_delta_cmp(const struct object_id *a, const struct object_id *b)\n \tkhiter_t a_pos, b_pos;\n \tstruct island_bitmap *a_bitmap = NULL, *b_bitmap = NULL;\n \n-\tif (!island_marks)\n+\tif (!using_island_marks)\n \t\treturn 0;\n \n \ta_pos = kh_get_oid_map(island_marks, *a);\n@@ -485,6 +486,7 @@ void load_delta_islands(struct repository *r, int progress)\n {\n \tstruct island_load_data ild = { 0 };\n \n+\tusing_island_marks = 1;\n \tisland_marks = kh_init_oid_map();\n \n \tgit_config(island_config_callback, &ild);\n@@ -522,6 +524,7 @@ static void free_island_marks(void)\n \t\t\tfree(bitmap);\n \t});\n \tkh_destroy_oid_map(island_marks);\n+\tisland_marks = NULL;\n }\n \n int compute_pack_layers(struct packing_data *to_pack)\n\nOf course it would be cleaner still if there was a \"struct\ndelta_islands\" that encapsulated the variables, and then the caller\ncould free it when they're done. That's more work to retro-fit, though.\n\n>  int compute_pack_layers(struct packing_data *to_pack)\n>  {\n>  \tuint32_t i;\n>  \n> -\tif (!core_island_name || !island_marks)\n> +\tif (!island_marks)\n> +\t\treturn 1;\n> +\n> +\tif (!core_island_name) {\n> +\t\tfree_island_marks();\n>  \t\treturn 1;\n> +\t}\n>  \n>  \tfor (i = 0; i < to_pack->nr_objects; ++i) {\n>  \t\tstruct object_entry *entry = &to_pack->objects[i];\n> @@ -533,6 +549,7 @@ int compute_pack_layers(struct packing_data *to_pack)\n>  \t\t\t\toe_set_layer(to_pack, entry, 0);\n>  \t\t}\n>  \t}\n> +\tfree_island_marks();\n>  \n>  \treturn 2;\n>  }\n\nThis is pretty subtle. It implicitly assumes that compute_pack_layers()\nis the last thing that will ever need to look at islands. Which is true\nnow (I think), but it's reasonable to think that somebody might use\nisland data during bitmap generation, too.\n\nIt seems like it would be a lot more obvious if the sole caller did the\nfree explicitly, like:\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex cabace4abb..3395f63aba 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -929,8 +929,10 @@ static struct object_entry **compute_write_order(void)\n \t */\n \tfor_each_tag_ref(mark_tagged, NULL);\n \n-\tif (use_delta_islands)\n+\tif (use_delta_islands) {\n \t\tmax_layers = compute_pack_layers(&to_pack);\n+\t\tfree_island_marks();\n+\t}\n \n \tALLOC_ARRAY(wo, to_pack.nr_objects);\n \two_end = 0;\n\nAnd of course that would also be a tiny step in the right direction if\nthe delta islands API learned to use a struct (this would be the same\nspot where we'd say \"we're done with islands; free the struct\").\n\n-Peff\n"},{"id":"471470","messageId":"20230203234430.M553381@dcvr","threadId":"59180","inReplyTo":"Y91OZHrMfca6tb/2@coredump.intra.peff.net","subject":"[PATCH v3] delta-islands: free island_marks and bitmaps","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2023-02-03T23:44:30Z","receivedAt":"2023-02-03T23:44:33Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King <peff@peff.net> wrote:\n> On Thu, Feb 02, 2023 at 09:42:17AM +0000, Eric Wong wrote:\n> \n> > > > +\t\t\tfree(bitmap);\n> > > > +\t});\n> > > > +\tkh_destroy_oid_map(island_marks);\n> > > > +\tisland_marks = (void *)1; /* crash on unintended future use */\n> > > \n> > > This seems counter-productive. If you just leave it free'd then various\n> > > analysis tools will spot a use-after-free earlier, won't they?\n> > \n> > *shrug*  I thought it might be better to use an explicitly bad\n> > pointer to catch lifetime issues that I might've missed from\n> > reading the code.  Since I've run and tested at this point,\n> > it probably doesn't matter, now.  So I'll just omit the\n> > assignment and save some icache footprint.\n> \n> I think it is still worth protecting ourselves. You've run and tested\n> the current code, but the risk remains that somebody will later change\n> the code and introduce a regression.\n> \n> Yes, analysis tools may spot it. But only if we actually trigger the\n> case in question via the test suite, and while running those tools. We\n> can get more consistent behavior by overwriting the pointer.\n\nOK.\n\n> The usual thing, of course, would just be to set it to NULL. I guess you\n> used \"(void *)1\" in the original because code like in_same_island()\n> checks for NULL. And so it becomes indistinguishable whether we intended\n> not to use islands, or if there's a bug in the program.\n\nYes, I used `(void *)1' because of the existing NULL checks.  I\nshould have used `(void *)-1' (MAP_FAILED) instead since\nattempts to dereference 1 could conceivably point to a valid\naddress, whereas using a maximum value would not.\n\n> If we are going to reduce the lifetime of island_marks, I think it would\n> make things cleaner to push the \"do we expect to have islands\" boolean\n> into its own variable, something like:\n> \n> diff --git a/delta-islands.c b/delta-islands.c\n> index c09dab31a4..7dd4e1419a 100644\n> --- a/delta-islands.c\n> +++ b/delta-islands.c\n> @@ -22,6 +22,7 @@\n>  \n>  KHASH_INIT(str, const char *, void *, 1, kh_str_hash_func, kh_str_hash_equal)\n>  \n> +static int using_island_marks;\n>  static kh_oid_map_t *island_marks;\n>  static unsigned island_counter;\n>  static unsigned island_counter_core;\n> @@ -96,7 +97,7 @@ int in_same_island(const struct object_id *trg_oid, const struct object_id *src_\n>  \tkhiter_t trg_pos, src_pos;\n>  \n>  \t/* If we aren't using islands, assume everything goes together. */\n> -\tif (!island_marks)\n> +\tif (!using_island_marks)\n>  \t\treturn 1;\n\nI much prefer to rely on invalid pointers than extra flags since\nhaving multiple sources of truth confuses me[1].\n\n> Of course it would be cleaner still if there was a \"struct\n> delta_islands\" that encapsulated the variables, and then the caller\n> could free it when they're done. That's more work to retro-fit, though.\n\nYes and yes :>\n\n> >  int compute_pack_layers(struct packing_data *to_pack)\n> >  {\n> >  \tuint32_t i;\n> >  \n> > -\tif (!core_island_name || !island_marks)\n> > +\tif (!island_marks)\n> > +\t\treturn 1;\n> > +\n> > +\tif (!core_island_name) {\n> > +\t\tfree_island_marks();\n> >  \t\treturn 1;\n> > +\t}\n> >  \n> >  \tfor (i = 0; i < to_pack->nr_objects; ++i) {\n> >  \t\tstruct object_entry *entry = &to_pack->objects[i];\n> > @@ -533,6 +549,7 @@ int compute_pack_layers(struct packing_data *to_pack)\n> >  \t\t\t\toe_set_layer(to_pack, entry, 0);\n> >  \t\t}\n> >  \t}\n> > +\tfree_island_marks();\n> >  \n> >  \treturn 2;\n> >  }\n> \n> This is pretty subtle. It implicitly assumes that compute_pack_layers()\n> is the last thing that will ever need to look at islands. Which is true\n> now (I think), but it's reasonable to think that somebody might use\n> island data during bitmap generation, too.\n\nYeah.  The reason I used an invalid pointer in the original was\nbecause I wasn't certain compute_pack_layers() was the last user.\n\n> It seems like it would be a lot more obvious if the sole caller did the\n> free explicitly, like:\n> \n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index cabace4abb..3395f63aba 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -929,8 +929,10 @@ static struct object_entry **compute_write_order(void)\n>  \t */\n>  \tfor_each_tag_ref(mark_tagged, NULL);\n>  \n> -\tif (use_delta_islands)\n> +\tif (use_delta_islands) {\n>  \t\tmax_layers = compute_pack_layers(&to_pack);\n> +\t\tfree_island_marks();\n> +\t}\n\nOK, I've done that for v3.  Thanks.\n\n>  \tALLOC_ARRAY(wo, to_pack.nr_objects);\n>  \two_end = 0;\n> \n> And of course that would also be a tiny step in the right direction if\n> the delta islands API learned to use a struct (this would be the same\n> spot where we'd say \"we're done with islands; free the struct\").\n\nI do wonder about performance on register-starved systems,\nthough, especially if stuff like island_delta_cmp gets called\nfrequently.  I already have enough performance problems atm :<\n\n[1] to go farther, I might even eliminate `int use_delta_islands' as\n    a global from builtin/pack-objects.c and just have that become a\n    `struct delta_islands_foo *' or something.  But I have more\n    pressing performance problems to figure out :<\n\n--------8<-------\nSubject: [PATCH v3] delta-islands: free island_marks and bitmaps\n\nOn my mirror of linux.git forkgroup with 780 islands, this saves\nnearly 4G of heap memory in pack-objects.  This savings only\nbenefits delta island users of pack bitmaps, as the process\nwould otherwise be exiting anyways.\n\nHowever, there's probably not many delta island users, but the\nmajority of delta island users would also be pack bitmaps users.\n\nSigned-off-by: Eric Wong <e@80x24.org>\nHelped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nHelped-by: Jeff King <peff@peff.net>\n---\n v3: use (void *)-1 to detect use-after-free,\n     hoist out free_island_marks() call for readability\n\n builtin/pack-objects.c |  4 +++-\n delta-islands.c        | 14 ++++++++++++++\n delta-islands.h        |  1 +\n 3 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex cabace4abb..3395f63aba 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -929,8 +929,10 @@ static struct object_entry **compute_write_order(void)\n \t */\n \tfor_each_tag_ref(mark_tagged, NULL);\n \n-\tif (use_delta_islands)\n+\tif (use_delta_islands) {\n \t\tmax_layers = compute_pack_layers(&to_pack);\n+\t\tfree_island_marks();\n+\t}\n \n \tALLOC_ARRAY(wo, to_pack.nr_objects);\n \two_end = 0;\ndiff --git a/delta-islands.c b/delta-islands.c\nindex 90c0d6958f..8b234cb85b 100644\n--- a/delta-islands.c\n+++ b/delta-islands.c\n@@ -513,6 +513,20 @@ void propagate_island_marks(struct commit *commit)\n \t}\n }\n \n+void free_island_marks(void)\n+{\n+\tstruct island_bitmap *bitmap;\n+\n+\tkh_foreach_value(island_marks, bitmap, {\n+\t\tif (!--bitmap->refcount)\n+\t\t\tfree(bitmap);\n+\t});\n+\tkh_destroy_oid_map(island_marks);\n+\n+\t/* detect use-after-free with a an address which is never valid: */\n+\tisland_marks = (void *)-1;\n+}\n+\n int compute_pack_layers(struct packing_data *to_pack)\n {\n \tuint32_t i;\ndiff --git a/delta-islands.h b/delta-islands.h\nindex eb0f952629..8d1591ae28 100644\n--- a/delta-islands.h\n+++ b/delta-islands.h\n@@ -14,5 +14,6 @@ void resolve_tree_islands(struct repository *r,\n void load_delta_islands(struct repository *r, int progress);\n void propagate_island_marks(struct commit *commit);\n int compute_pack_layers(struct packing_data *to_pack);\n+void free_island_marks(void);\n \n #endif /* DELTA_ISLANDS_H */\n"},{"id":"471478","messageId":"Y94+L/mhBnnbi+0k@coredump.intra.peff.net","threadId":"59180","inReplyTo":"20230203234430.M553381@dcvr","subject":"Re: [PATCH v3] delta-islands: free island_marks and bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-04T11:14:55Z","receivedAt":"2023-02-04T11:15:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 03, 2023 at 11:44:30PM +0000, Eric Wong wrote:\n\n> >  \t/* If we aren't using islands, assume everything goes together. */\n> > -\tif (!island_marks)\n> > +\tif (!using_island_marks)\n> >  \t\treturn 1;\n> \n> I much prefer to rely on invalid pointers than extra flags since\n> having multiple sources of truth confuses me[1].\n\nThat's kind of my point, though. It's not multiple sources of truth, but\nrather there are two bits \"did we ask to use islands\" and \"is\nisland_marks still valid\". You are shoving both bits into the same\nvariable by using a special sentinel pointer.\n\n> > And of course that would also be a tiny step in the right direction if\n> > the delta islands API learned to use a struct (this would be the same\n> > spot where we'd say \"we're done with islands; free the struct\").\n> \n> I do wonder about performance on register-starved systems,\n> though, especially if stuff like island_delta_cmp gets called\n> frequently.  I already have enough performance problems atm :<\n\nCalling in_same_island() is pretty heavy-weight (it's multiple hash\nlookups, and then an arbitrary-length bit-string comparison). I'd be\nshocked if replacing a global with a struct pointer is even measurable.\n\n> [1] to go farther, I might even eliminate `int use_delta_islands' as\n>     a global from builtin/pack-objects.c and just have that become a\n>     `struct delta_islands_foo *' or something.  But I have more\n>     pressing performance problems to figure out :<\n\nRight, that's along the same lines I was thinking. But I don't blame\nyou for not tackling it. The upside is fairly minimal.\n\n> +\t/* detect use-after-free with a an address which is never valid: */\n> +\tisland_marks = (void *)-1;\n\nI still hate how magical this line is, but I don't that it's worth\narguing about more.\n\n-Peff\n"}]}