(no longer active) Assignee
	Description • 11 years ago

The uconv code consists of a number of small C++ compilation units which are
easy to convert to a unified build.  On my machine, this speeds up the
compilation of this directory by 4 seconds on 8 cores.

https://tbpl.mozilla.org/?tree=Try&rev=5da048668239

	(no longer active) Assignee
	Comment 2 • 11 years ago

Comment on attachment 818836 [details] [diff] [review]
Build the uconv library in unified mode

Review of attachment 818836 [details] [diff] [review]:
-----------------------------------------------------------------

(nsUnicodeToISO2022CN.* were two empty files which I removed in this patch, but git's diff is confused about them.  I'll create a proper patch before landing.)

	Mike Hommey [:glandium]
	Comment 3 • 11 years ago

Comment on attachment 818836 [details] [diff] [review]
Build the uconv library in unified mode

Review of attachment 818836 [details] [diff] [review]:
-----------------------------------------------------------------

::: intl/uconv/util/moz.build
@@ +6,5 @@
>  
>  MODULE = 'uconv'
>  
>  CPP_SOURCES += [
> +    'UnifiedUCVUtils.cpp',

I think it would be better to create those automatically from CPP_SOURCES. That avoids having to edit the unified file when adding or removing cpp files, instead of editing CPP_SOURCES like everywhere else.

In fact, I already have a patch with rules to do that.

	(no longer active) Assignee
	Comment 4 • 11 years ago

(In reply to comment #3)
> Comment on attachment 818836 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=818836
> Build the uconv library in unified mode
> 
> Review of attachment 818836 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: intl/uconv/util/moz.build
> @@ +6,5 @@
> >  
> >  MODULE = 'uconv'
> >  
> >  CPP_SOURCES += [
> > +    'UnifiedUCVUtils.cpp',
> 
> I think it would be better to create those automatically from CPP_SOURCES. That
> avoids having to edit the unified file when adding or removing cpp files,
> instead of editing CPP_SOURCES like everywhere else.

That's a bad idea since you may not want to build some of your files in Unified mode if they conflict with each other in unfixable ways.  This is not and will never be an automated transition and it is best to leave the decision on which files are built in Unified mode and how to the developers.

	Mike Hommey [:glandium]
	Comment 5 • 11 years ago

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #4)
> That's a bad idea since you may not want to build some of your files in
> Unified mode if they conflict with each other in unfixable ways.  This is
> not and will never be an automated transition and it is best to leave the
> decision on which files are built in Unified mode and how to the developers.

Which is why i want to add a UNIFIED_CPP_SOURCES variable.

	(no longer active) Assignee
	Comment 6 • 11 years ago

(In reply to Mike Hommey [:glandium] from comment #5)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #4)
> > That's a bad idea since you may not want to build some of your files in
> > Unified mode if they conflict with each other in unfixable ways.  This is
> > not and will never be an automated transition and it is best to leave the
> > decision on which files are built in Unified mode and how to the developers.
> 
> Which is why i want to add a UNIFIED_CPP_SOURCES variable.

OK, but that doesn't need to block this bug.  :-)

	Simon Montagu :smontagu
	Comment 7 • 11 years ago

Comment on attachment 818836 [details] [diff] [review]
Build the uconv library in unified mode

Review of attachment 818836 [details] [diff] [review]:
-----------------------------------------------------------------

If you're already doing this, would it make sense to put all the converters in a single directory? The current directory structure dates back to when there were separate libraries for each group of encodings, and there's no real need to preserve it as far as I can see.

And if you already do /that/, maybe it would be more efficient to regroup the sources to have encoders in one group and decoders in another (and if you want further subdivision you could separate out the multitable en- and de- coders and the algorithmic ones from the simple table ones?

	(no longer active) Assignee
	Comment 8 • 11 years ago

(In reply to Simon Montagu :smontagu from comment #7)
> Comment on attachment 818836 [details] [diff] [review]
> Build the uconv library in unified mode
> 
> Review of attachment 818836 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If you're already doing this, would it make sense to put all the converters
> in a single directory? The current directory structure dates back to when
> there were separate libraries for each group of encodings, and there's no
> real need to preserve it as far as I can see.

Hmm, I guess so but I would prefer to do that in a separate bug.

> And if you already do /that/, maybe it would be more efficient to regroup
> the sources to have encoders in one group and decoders in another (and if
> you want further subdivision you could separate out the multitable en- and
> de- coders and the algorithmic ones from the simple table ones?

My ultimate goal is to unify all of this code in one single cpp file, but I'd like to make baby steps towards that goal, since there is a bit of risk here, and I would like to create better bisection opportunity by moving slowly on this.

	Gregory Szorc [:gps]
	Comment 9 • 11 years ago

Comment on attachment 818836 [details] [diff] [review]
Build the uconv library in unified mode

Review of attachment 818836 [details] [diff] [review]:
-----------------------------------------------------------------

The way I see it, if you want to structure your C++ files this way without the cooperation of the build system, you are free to do that and don't need build module involvement. (As long as you don't break incremental builds, etc.)

That being said, checking in unified sources can lead to some gotchas. Not having the original .cpp files annotated in moz.build will also lead to suboptimal IDE project files (when we eventually produce those).

Long term, I imagine you'll want to do this properly, with build system integration. But, don't let the build system block on progress.

I'm removing myself from review as this is a uconv module decision (for now). That being said, I would encourage you to wait for the generic unified build mode support that glandium referenced.

	(no longer active) Assignee
	Comment 10 • 11 years ago

(In reply to Gregory Szorc [:gps] from comment #9)
> The way I see it, if you want to structure your C++ files this way without
> the cooperation of the build system, you are free to do that and don't need
> build module involvement. (As long as you don't break incremental builds,
> etc.)

Yeah, I was mostly using the review flag as a way to communicate this change to you.  :-)

> That being said, checking in unified sources can lead to some gotchas.

Like what?

> Not
> having the original .cpp files annotated in moz.build will also lead to
> suboptimal IDE project files (when we eventually produce those).

All IDEs that I know of are capable of handling #include's.  That's needed for pretty much anything interesting that you may want to do in the IDE.

> Long term, I imagine you'll want to do this properly, with build system
> integration. But, don't let the build system block on progress.

Sure!

> I'm removing myself from review as this is a uconv module decision (for
> now). That being said, I would encourage you to wait for the generic unified
> build mode support that glandium referenced.

It's not quite clear to me when that will land.  If that's in a few days, I can wait, but I'm not inclined to do that if it's going to take more time, since essentially that will make me rewrite half of this patch.  I guess whoever lands second will get to deal with that though.  :-)

	Simon Montagu :smontagu
	Comment 11 • 11 years ago

Comment on attachment 818836 [details] [diff] [review]
Build the uconv library in unified mode

Review of attachment 818836 [details] [diff] [review]:
-----------------------------------------------------------------

I'll buy baby steps :)

	(no longer active) Assignee
	Comment 12 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/833a5441ef8b

	(no longer active) Assignee
	Comment 13 • 11 years ago

Pushed a follow-up to rename the SIZE_OF_TABLES macro.

	Phil Ringnalda (:philor)
	Comment 14 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/833a5441ef8b
https://hg.mozilla.org/mozilla-central/rev/8fcdb3653f57