* [PATCH] Add --no-hard-links option to localedef (Bug 23923)
@ 2018-11-26 15:21 Carlos O'Donell
2018-11-26 16:05 ` Florian Weimer
2018-11-26 16:59 ` Joseph Myers
0 siblings, 2 replies; 7+ messages in thread
From: Carlos O'Donell @ 2018-11-26 15:21 UTC (permalink / raw)
To: GNU C Library, Florian Weimer
[PATCH] Add --no-hard-links option to localedef (bug 23923)
Downstream distributions need consistent sets of hardlinks in
order for rpm to operate effectively. This means that even if
locales are built with a high level of parallelism that the
resulting files need to have consistent hardlink counts. The only
way to achieve this is with a post-install hardlink pass using a
program like 'hardklink' (shipped in Fedora).
If the downstream distro wants to post-process the hardlinks then
the time spent in localedef looking up sibling directories and
processing hardlinks is wasted effort.
To optimize the build and install pass we add a --no-hard-links
option to localedef to avoid doing the hardlink optimziation for
size.
Tested on x86_64 with 'make localedata/install-locale-files'
before and after. Without the patch we have files with 70+
hardlink counts. After the patch and running with --no-hard-links
all link counts are 1.
Signed-off-by: Carlos O'Donell <carlos@redhat.com>
---
ChangeLog | 10 ++++++++++
locale/programs/localedef.c | 10 ++++++++++
locale/programs/localedef.h | 1 +
locale/programs/locfile.c | 16 +++++++++++++---
4 files changed, 34 insertions(+), 3 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 03887874a2..b9bce8bc8e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2018-11-26 Carlos O'Donell <carlos@redhat.com>
+
+ [BZ #23923]
+ * locale/programs/localedef.c: Declare boolean hard_links default true.
+ (options): Add --no-hard-links option.
+ (parse_opt): Add OPT_NO_HARD_LINKS case and set hard_links to false.
+ * locale/programs/localedef.h: Declare prototype for hard_links.
+ * locale/programs/locfile.c (write_locale_data): Don't use hard
+ links if hard_links is false.
+
2018-11-26 Carlos O'Donell <carlos@redhat.com>
* scripts/abilist.awk: Handle .tdata. Error for unknown combinations.
diff --git a/locale/programs/localedef.c b/locale/programs/localedef.c
index d718d2e9f4..6c4936be6b 100644
--- a/locale/programs/localedef.c
+++ b/locale/programs/localedef.c
@@ -85,6 +85,9 @@ static bool replace_archive;
/* If true list archive content. */
static bool list_archive;
+/* If true create hard links to other locales (default). */
+bool hard_links = true;
+
/* Maximum number of retries when opening the locale archive. */
int max_locarchive_open_retry = 10;
@@ -105,6 +108,7 @@ void (*argp_program_version_hook) (FILE *, struct argp_state *) = print_version;
#define OPT_BIG_ENDIAN 401
#define OPT_NO_WARN 402
#define OPT_WARN 403
+#define OPT_NO_HARD_LINKS 404
/* Definitions of arguments for argp functions. */
static const struct argp_option options[] =
@@ -120,6 +124,8 @@ static const struct argp_option options[] =
{ NULL, 0, NULL, 0, N_("Output control:") },
{ "force", 'c', NULL, 0,
N_("Create output even if warning messages were issued") },
+ { "no-hard-links", OPT_NO_HARD_LINKS, NULL, 0,
+ N_("Do not create hard links between installed locales") },
{ "prefix", OPT_PREFIX, N_("PATH"), 0, N_("Optional output file prefix") },
{ "posix", OPT_POSIX, NULL, 0, N_("Strictly conform to POSIX") },
{ "quiet", OPT_QUIET, NULL, 0,
@@ -389,6 +395,10 @@ parse_opt (int key, char *arg, struct argp_state *state)
/* Enable the warnings. */
set_warnings (arg, true);
break;
+ case OPT_NO_HARD_LINKS:
+ /* Do not hard link to other locales. */
+ hard_links = false;
+ break;
case 'c':
force_output = 1;
break;
diff --git a/locale/programs/localedef.h b/locale/programs/localedef.h
index 0083faceab..e2b39e78f3 100644
--- a/locale/programs/localedef.h
+++ b/locale/programs/localedef.h
@@ -118,6 +118,7 @@ extern const char *repertoire_global;
extern int max_locarchive_open_retry;
extern bool no_archive;
extern const char *alias_file;
+extern bool hard_links;
/* Prototypes for a few program-wide used functions. */
diff --git a/locale/programs/locfile.c b/locale/programs/locfile.c
index 32e5f761f2..14c743373d 100644
--- a/locale/programs/locfile.c
+++ b/locale/programs/locfile.c
@@ -702,7 +702,7 @@ write_locale_data (const char *output_path, int catidx, const char *category,
size_t cnt, step, maxiov;
int fd;
char *fname;
- const char **other_paths;
+ const char **other_paths = NULL;
uint32_t header[2];
size_t n_elem;
struct iovec vec[3];
@@ -828,8 +828,18 @@ failure while writing data for category `%s'"), category);
close (fd);
/* Compare the file with the locale data files for the same category in
- other locales, and see if we can reuse it, to save disk space. */
- other_paths = siblings (output_path);
+ other locales, and see if we can reuse it, to save disk space.
+ If the user specified --no-hard-link to localedef then hard_links is
+ false, other_paths remains NULL and we skip the optimization below.
+ The use of --no-hard-link is distribution specific since some distros
+ have post-processing hard-link steps and so doing this here is a waste
+ of time. */
+ if (hard_links)
+ other_paths = siblings (output_path);
+
+ /* If there are other paths, then walk the sibling paths looking for
+ files with the same content so we can hard link and reduce disk
+ space usage. */
if (other_paths != NULL)
{
struct stat64 fname_stat;
--
2.19.1
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Add --no-hard-links option to localedef (Bug 23923)
2018-11-26 15:21 [PATCH] Add --no-hard-links option to localedef (Bug 23923) Carlos O'Donell
@ 2018-11-26 16:05 ` Florian Weimer
2018-11-26 16:59 ` Joseph Myers
1 sibling, 0 replies; 7+ messages in thread
From: Florian Weimer @ 2018-11-26 16:05 UTC (permalink / raw)
To: Carlos O'Donell; +Cc: GNU C Library
* Carlos O'Donell:
> + [BZ #23923]
> + * locale/programs/localedef.c: Declare boolean hard_links default true.
> + (options): Add --no-hard-links option.
> + (parse_opt): Add OPT_NO_HARD_LINKS case and set hard_links to false.
> + * locale/programs/localedef.h: Declare prototype for hard_links.
> + * locale/programs/locfile.c (write_locale_data): Don't use hard
> + links if hard_links is false.
Looks reasonable to me.
This will need a makefile update, so that we can use this from “make
install-locales”.
Thanks,
Florian
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Add --no-hard-links option to localedef (Bug 23923)
2018-11-26 15:21 [PATCH] Add --no-hard-links option to localedef (Bug 23923) Carlos O'Donell
2018-11-26 16:05 ` Florian Weimer
@ 2018-11-26 16:59 ` Joseph Myers
2018-11-26 19:25 ` [PATCHv2] " Carlos O'Donell
1 sibling, 1 reply; 7+ messages in thread
From: Joseph Myers @ 2018-11-26 16:59 UTC (permalink / raw)
To: Carlos O'Donell; +Cc: GNU C Library, Florian Weimer
On Mon, 26 Nov 2018, Carlos O'Donell wrote:
> If the downstream distro wants to post-process the hardlinks then
> the time spent in localedef looking up sibling directories and
> processing hardlinks is wasted effort.
Do you have figures for the time involved?
> + other locales, and see if we can reuse it, to save disk space.
> + If the user specified --no-hard-link to localedef then hard_links is
> + false, other_paths remains NULL and we skip the optimization below.
> + The use of --no-hard-link is distribution specific since some distros
> + have post-processing hard-link steps and so doing this here is a waste
> + of time. */
This should refer to --no-hard-links, not --no-hard-link (twice).
This is not a review of other aspects of the patch (but I approve of
improving determinism of the locale build, and also have such a hard-link
post-processing step at Mentor).
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCHv2] Add --no-hard-links option to localedef (Bug 23923)
2018-11-26 16:59 ` Joseph Myers
@ 2018-11-26 19:25 ` Carlos O'Donell
2018-11-26 19:27 ` Florian Weimer
0 siblings, 1 reply; 7+ messages in thread
From: Carlos O'Donell @ 2018-11-26 19:25 UTC (permalink / raw)
To: Joseph Myers; +Cc: GNU C Library, Florian Weimer
On 11/26/18 11:58 AM, Joseph Myers wrote:
> On Mon, 26 Nov 2018, Carlos O'Donell wrote:
>
>> If the downstream distro wants to post-process the hardlinks then
>> the time spent in localedef looking up sibling directories and
>> processing hardlinks is wasted effort.
>
> Do you have figures for the time involved?
On a fast SSD the -j1 locale build and install of SUPPORTED locales
is 3% faster, or for me roughly ~10s on a:
"make localedata/install-locale-files DESTDIR=..." iteration.
The option itself ensures that no hard links are present and that may
make the logic of the post-install hardlink creation phase simpler.
So there is more than just performance to consider.
>> + other locales, and see if we can reuse it, to save disk space.
>> + If the user specified --no-hard-link to localedef then hard_links is
>> + false, other_paths remains NULL and we skip the optimization below.
>> + The use of --no-hard-link is distribution specific since some distros
>> + have post-processing hard-link steps and so doing this here is a waste
>> + of time. */
>
> This should refer to --no-hard-links, not --no-hard-link (twice).
Fixed.
> This is not a review of other aspects of the patch (but I approve of
> improving determinism of the locale build, and also have such a hard-link
> post-processing step at Mentor).
v2
- Fix comments to mention '--no-hard-links'
- Update convenience target to use '--no-hard-links'
[PATCH] Add --no-hard-links option to localedef (bug 23923)
Downstream distributions need consistent sets of hardlinks in
order for rpm to operate effectively. This means that even if
locales are built with a high level of parallelism that the
resulting files need to have consistent hardlink counts. The only
way to achieve this is with a post-install hardlink pass using a
program like 'hardklink' (shipped in Fedora).
If the downstream distro wants to post-process the hardlinks then
the time spent in localedef looking up sibling directories and
processing hardlinks is wasted effort.
To optimize the build and install pass we add a --no-hard-links
option to localedef to avoid doing the hardlink optimziation for
size.
Tested on x86_64 with 'make localedata/install-locale-files'
before and after. Without the patch we have files with 100+
hardlink counts. After the patch and running with --no-hard-links
all link counts are 1. This patch also alters the convenience
target 'make localedata/install-locale-files' to use the new
option.
Signed-off-by: Carlos O'Donell <carlos@redhat.com>
---
ChangeLog | 10 ++++++++++
locale/programs/localedef.c | 10 ++++++++++
locale/programs/localedef.h | 1 +
locale/programs/locfile.c | 16 +++++++++++++---
localedata/Makefile | 2 +-
5 files changed, 35 insertions(+), 4 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 03887874a2..b9bce8bc8e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2018-11-26 Carlos O'Donell <carlos@redhat.com>
+
+ [BZ #23923]
+ * locale/programs/localedef.c: Declare boolean hard_links default true.
+ (options): Add --no-hard-links option.
+ (parse_opt): Add OPT_NO_HARD_LINKS case and set hard_links to false.
+ * locale/programs/localedef.h: Declare prototype for hard_links.
+ * locale/programs/locfile.c (write_locale_data): Don't use hard
+ links if hard_links is false.
+
2018-11-26 Carlos O'Donell <carlos@redhat.com>
* scripts/abilist.awk: Handle .tdata. Error for unknown combinations.
diff --git a/locale/programs/localedef.c b/locale/programs/localedef.c
index d718d2e9f4..6c4936be6b 100644
--- a/locale/programs/localedef.c
+++ b/locale/programs/localedef.c
@@ -85,6 +85,9 @@ static bool replace_archive;
/* If true list archive content. */
static bool list_archive;
+/* If true create hard links to other locales (default). */
+bool hard_links = true;
+
/* Maximum number of retries when opening the locale archive. */
int max_locarchive_open_retry = 10;
@@ -105,6 +108,7 @@ void (*argp_program_version_hook) (FILE *, struct argp_state *) = print_version;
#define OPT_BIG_ENDIAN 401
#define OPT_NO_WARN 402
#define OPT_WARN 403
+#define OPT_NO_HARD_LINKS 404
/* Definitions of arguments for argp functions. */
static const struct argp_option options[] =
@@ -120,6 +124,8 @@ static const struct argp_option options[] =
{ NULL, 0, NULL, 0, N_("Output control:") },
{ "force", 'c', NULL, 0,
N_("Create output even if warning messages were issued") },
+ { "no-hard-links", OPT_NO_HARD_LINKS, NULL, 0,
+ N_("Do not create hard links between installed locales") },
{ "prefix", OPT_PREFIX, N_("PATH"), 0, N_("Optional output file prefix") },
{ "posix", OPT_POSIX, NULL, 0, N_("Strictly conform to POSIX") },
{ "quiet", OPT_QUIET, NULL, 0,
@@ -389,6 +395,10 @@ parse_opt (int key, char *arg, struct argp_state *state)
/* Enable the warnings. */
set_warnings (arg, true);
break;
+ case OPT_NO_HARD_LINKS:
+ /* Do not hard link to other locales. */
+ hard_links = false;
+ break;
case 'c':
force_output = 1;
break;
diff --git a/locale/programs/localedef.h b/locale/programs/localedef.h
index 0083faceab..e2b39e78f3 100644
--- a/locale/programs/localedef.h
+++ b/locale/programs/localedef.h
@@ -118,6 +118,7 @@ extern const char *repertoire_global;
extern int max_locarchive_open_retry;
extern bool no_archive;
extern const char *alias_file;
+extern bool hard_links;
/* Prototypes for a few program-wide used functions. */
diff --git a/locale/programs/locfile.c b/locale/programs/locfile.c
index 32e5f761f2..c75eabc886 100644
--- a/locale/programs/locfile.c
+++ b/locale/programs/locfile.c
@@ -702,7 +702,7 @@ write_locale_data (const char *output_path, int catidx, const char *category,
size_t cnt, step, maxiov;
int fd;
char *fname;
- const char **other_paths;
+ const char **other_paths = NULL;
uint32_t header[2];
size_t n_elem;
struct iovec vec[3];
@@ -828,8 +828,18 @@ failure while writing data for category `%s'"), category);
close (fd);
/* Compare the file with the locale data files for the same category in
- other locales, and see if we can reuse it, to save disk space. */
- other_paths = siblings (output_path);
+ other locales, and see if we can reuse it, to save disk space.
+ If the user specified --no-hard-links to localedef then hard_links is
+ false, other_paths remains NULL and we skip the optimization below.
+ The use of --no-hard-links is distribution specific since some distros
+ have post-processing hard-link steps and so doing this here is a waste
+ of time. */
+ if (hard_links)
+ other_paths = siblings (output_path);
+
+ /* If there are other paths, then walk the sibling paths looking for
+ files with the same content so we can hard link and reduce disk
+ space usage. */
if (other_paths != NULL)
{
struct stat64 fname_stat;
diff --git a/localedata/Makefile b/localedata/Makefile
index 0eea396ad8..b245f0ff54 100644
--- a/localedata/Makefile
+++ b/localedata/Makefile
@@ -423,7 +423,7 @@ $(INSTALL-SUPPORTED-LOCALE-ARCHIVE): install-locales-dir
$(build-one-locale)
$(INSTALL-SUPPORTED-LOCALE-FILES): install-locales-dir
- @flags="-c --no-archive"; \
+ @flags="-c --no-archive --no-hard-links"; \
$(build-one-locale)
tst-setlocale-ENV = LC_ALL=ja_JP.EUC-JP
--
2.19.1
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCHv2] Add --no-hard-links option to localedef (Bug 23923)
2018-11-26 19:25 ` [PATCHv2] " Carlos O'Donell
@ 2018-11-26 19:27 ` Florian Weimer
2018-11-26 19:33 ` Carlos O'Donell
0 siblings, 1 reply; 7+ messages in thread
From: Florian Weimer @ 2018-11-26 19:27 UTC (permalink / raw)
To: Carlos O'Donell; +Cc: Joseph Myers, GNU C Library
* Carlos O'Donell:
> On 11/26/18 11:58 AM, Joseph Myers wrote:
>> On Mon, 26 Nov 2018, Carlos O'Donell wrote:
>>
>>> If the downstream distro wants to post-process the hardlinks then
>>> the time spent in localedef looking up sibling directories and
>>> processing hardlinks is wasted effort.
>>
>> Do you have figures for the time involved?
>
> On a fast SSD the -j1 locale build and install of SUPPORTED locales
> is 3% faster, or for me roughly ~10s on a:
> "make localedata/install-locale-files DESTDIR=..." iteration.
>
> The option itself ensures that no hard links are present and that may
> make the logic of the post-install hardlink creation phase simpler.
> So there is more than just performance to consider.
The real performance win comes from the ability to use parallel make and
still get fully deterministic results.
Thanks,
Florian
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCHv2] Add --no-hard-links option to localedef (Bug 23923)
2018-11-26 19:27 ` Florian Weimer
@ 2018-11-26 19:33 ` Carlos O'Donell
2018-12-03 15:22 ` Carlos O'Donell
0 siblings, 1 reply; 7+ messages in thread
From: Carlos O'Donell @ 2018-11-26 19:33 UTC (permalink / raw)
To: Florian Weimer; +Cc: Joseph Myers, GNU C Library
On 11/26/18 2:26 PM, Florian Weimer wrote:
> * Carlos O'Donell:
>
>> On 11/26/18 11:58 AM, Joseph Myers wrote:
>>> On Mon, 26 Nov 2018, Carlos O'Donell wrote:
>>>
>>>> If the downstream distro wants to post-process the hardlinks then
>>>> the time spent in localedef looking up sibling directories and
>>>> processing hardlinks is wasted effort.
>>>
>>> Do you have figures for the time involved?
>>
>> On a fast SSD the -j1 locale build and install of SUPPORTED locales
>> is 3% faster, or for me roughly ~10s on a:
>> "make localedata/install-locale-files DESTDIR=..." iteration.
>>
>> The option itself ensures that no hard links are present and that may
>> make the logic of the post-install hardlink creation phase simpler.
>> So there is more than just performance to consider.
>
> The real performance win comes from the ability to use parallel make and
> still get fully deterministic results.
If you are OK with v2 then I'll give it a few days for others to comment
and then commit. In v2 I switch the 'make localedata/install-locale-files'
to use --no-hard-links as the default distribution way to install the
files deterministically regardless of build paralleism.
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCHv2] Add --no-hard-links option to localedef (Bug 23923)
2018-11-26 19:33 ` Carlos O'Donell
@ 2018-12-03 15:22 ` Carlos O'Donell
0 siblings, 0 replies; 7+ messages in thread
From: Carlos O'Donell @ 2018-12-03 15:22 UTC (permalink / raw)
To: Florian Weimer; +Cc: Joseph Myers, GNU C Library
On 11/26/18 2:33 PM, Carlos O'Donell wrote:
> On 11/26/18 2:26 PM, Florian Weimer wrote:
>> * Carlos O'Donell:
>>
>>> On 11/26/18 11:58 AM, Joseph Myers wrote:
>>>> On Mon, 26 Nov 2018, Carlos O'Donell wrote:
>>>>
>>>>> If the downstream distro wants to post-process the hardlinks then
>>>>> the time spent in localedef looking up sibling directories and
>>>>> processing hardlinks is wasted effort.
>>>>
>>>> Do you have figures for the time involved?
>>>
>>> On a fast SSD the -j1 locale build and install of SUPPORTED locales
>>> is 3% faster, or for me roughly ~10s on a:
>>> "make localedata/install-locale-files DESTDIR=..." iteration.
>>>
>>> The option itself ensures that no hard links are present and that may
>>> make the logic of the post-install hardlink creation phase simpler.
>>> So there is more than just performance to consider.
>>
>> The real performance win comes from the ability to use parallel make and
>> still get fully deterministic results.
>
> If you are OK with v2 then I'll give it a few days for others to comment
> and then commit. In v2 I switch the 'make localedata/install-locale-files'
> to use --no-hard-links as the default distribution way to install the
> files deterministically regardless of build paralleism.
>
OK, I've pushed v2, and this alters 'make localedata/install-locale-files'
I have also added a note in the "Packaging Changes" for distribution maintainers
to read regarding the change in the convenience target:
https://sourceware.org/glibc/wiki/Release/2.29#Localedata_installation_without_hard_links
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-12-03 15:22 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-26 15:21 [PATCH] Add --no-hard-links option to localedef (Bug 23923) Carlos O'Donell
2018-11-26 16:05 ` Florian Weimer
2018-11-26 16:59 ` Joseph Myers
2018-11-26 19:25 ` [PATCHv2] " Carlos O'Donell
2018-11-26 19:27 ` Florian Weimer
2018-11-26 19:33 ` Carlos O'Donell
2018-12-03 15:22 ` Carlos O'Donell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).