322: Enhance AOT compilation process to emit classfiles only for explicitly-specified namespaces

12 views
Skip to first unread message

Chas Emerick

unread,
Oct 28, 2010, 12:09:11 PM10/28/10
to cloju...@googlegroups.com
Forking from the "Improving the dependency story…" thread on a slight tangent:

On Oct 28, 2010, at 10:30 AM, Stuart Halloway wrote:

> (5) All: eliminating compiled binary releases wherever source will do, and identifying the things that force people to compile so that we can eliminate them.

I heard directly from no fewer than 5 people at the Conj that this behaviour is a major issue:

http://dev.clojure.org/jira/browse/CLJ-322

…to the extent that people are running a typical build process, but then figuring out which class files aren't related to project-local sources, and deleting them. Such insanity must stop.

The above ticket references a patch that has been verified by at least others. Rich, I guess you're not fond of the details of the patch; in any case, this is a serious problem for many, and any TLC that can be provided would be much appreciated.

Cheers,

- Chas

Rich Hickey

unread,
Oct 28, 2010, 5:08:06 PM10/28/10
to cloju...@googlegroups.com

Not at all, I haven't looked at it extensively. I was only reluctant
to slide it into 1.2 at the 11th hour. It's on the release.next track
and needs someone to screen it. I'm ok also with changing the default.

Rich

Chas Emerick

unread,
Nov 1, 2010, 5:04:57 AM11/1/10
to cloju...@googlegroups.com

On Oct 28, 2010, at 5:08 PM, Rich Hickey wrote:

>> The above ticket references a patch that has been verified by at least others. Rich, I guess you're not fond of the details of the patch; in any case, this is a serious problem for many, and any TLC that can be provided would be much appreciated.
>>
>
> Not at all, I haven't looked at it extensively. I was only reluctant to slide it into 1.2 at the 11th hour. It's on the release.next track and needs someone to screen it. I'm ok also with changing the default.

Okay, I guess I read too much into Stu's comment. :-)

I suppose I shouldn't be screening my own patch; so, if a kind soul would like to do a good deed, do take a look at CLJ-322:

http://dev.clojure.org/jira/browse/CLJ-322

Thanks,

- Chas

Cosmin Stejerean

unread,
Nov 1, 2010, 5:58:26 AM11/1/10
to cloju...@googlegroups.com
On Mon, Nov 1, 2010 at 5:04 AM, Chas Emerick <ceme...@snowtide.com> wrote:
> I suppose I shouldn't be screening my own patch; so, if a kind soul would like to do a good deed, do take a look at CLJ-322:
>
> http://dev.clojure.org/jira/browse/CLJ-322
>

I think we should turn off transitive compilation by default, rather
than providing the option to turn it off. The current behavior is
broken IMHO and should not remain the default.

The patch looks good, although it needs to be updated to work with the
new dynamic behavior. Specifically, adding setDynamic() to LOAD_LEVEL
and TRANSITIVE_COMPILE in Compiler.java

I do not however understand the changes to *warn-on-reflection*,
specifically the following in main.clj

- *warn-on-reflection* *warn-on-reflection*
+ *warn-on-reflection* (= "true" (System/getProperty
Compile/REFLECTION_WARNING_PROP "false"))

and in Compiler.java

-private static final String REFLECTION_WARNING_PROP =
"clojure.compile.warn-on-reflection";
+public static final String REFLECTION_WARNING_PROP =
"clojure.compile.warn-on-reflection";


Right now it looks like something unrelated that managed to get
captured in the patch, as I cannot tell what else about the patch
requires this. What am I missing?

--
Cosmin Stejerean
http://offbytwo.com

Stuart Halloway

unread,
Nov 1, 2010, 7:15:26 AM11/1/10
to cloju...@googlegroups.com
Cosmin: thanks for reviewing the ticket.

Everyone: *Please* do reviews like this on the ticket. You should be able to create your own account on JIRA.

And yes, I am going to keep responding to every thread this way until new habits take hold. :-)

Stu

> --
> You received this message because you are subscribed to the Google Groups "Clojure Dev" group.
> To post to this group, send email to cloju...@googlegroups.com.
> To unsubscribe from this group, send email to clojure-dev...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/clojure-dev?hl=en.
>

Cosmin Stejerean

unread,
Nov 1, 2010, 3:23:50 PM11/1/10
to cloju...@googlegroups.com
On Mon, Nov 1, 2010 at 7:15 AM, Stuart Halloway
<stuart....@gmail.com> wrote:
> You should be able to create your own account on JIRA

I can't figure out where I can do that. All I see is Log In, and I
tried logging in, and I also tried the forgot password / username
route with no luck. Where can I register for a Jira account?

Chas Emerick

unread,
Nov 2, 2010, 7:40:45 AM11/2/10
to cloju...@googlegroups.com

On Nov 1, 2010, at 5:58 AM, Cosmin Stejerean wrote:

> On Mon, Nov 1, 2010 at 5:04 AM, Chas Emerick <ceme...@snowtide.com> wrote:
>> I suppose I shouldn't be screening my own patch; so, if a kind soul would like to do a good deed, do take a look at CLJ-322:
>>
>> http://dev.clojure.org/jira/browse/CLJ-322
>>
>
> I think we should turn off transitive compilation by default, rather
> than providing the option to turn it off. The current behavior is
> broken IMHO and should not remain the default.

There are two patches on the ticket, one with transitive off by default, another that leaves the default untouched. I think the comment import from assembla led to some odd ordering issues, so that is probably the source of the mixup.

> The patch looks good, although it needs to be updated to work with the
> new dynamic behavior. Specifically, adding setDynamic() to LOAD_LEVEL
> and TRANSITIVE_COMPILE in Compiler.java

That's totally my bad. I should have realized this before pushing the issue. Will fix.

I'll post again when I've attached an improved patch.

Thanks for screening! :-)

- Chas

Chas Emerick

unread,
Nov 19, 2010, 11:27:06 AM11/19/10
to cloju...@googlegroups.com

On Nov 1, 2010, at 3:23 PM, Cosmin Stejerean wrote:

> On Mon, Nov 1, 2010 at 7:15 AM, Stuart Halloway
> <stuart....@gmail.com> wrote:
>> You should be able to create your own account on JIRA
>
> I can't figure out where I can do that. All I see is Log In, and I
> tried logging in, and I also tried the forgot password / username
> route with no luck. Where can I register for a Jira account?

There's a "sign up" link on the login page:

http://dev.clojure.org/jira/login.jsp?os_destination=%2Fsecure%2FDashboard.jspa

;-)

- Chas

Chas Emerick

unread,
Nov 19, 2010, 10:39:10 PM11/19/10
to cloju...@googlegroups.com

On Nov 1, 2010, at 5:58 AM, Cosmin Stejerean wrote:

> The patch looks good, although it needs to be updated to work with the
> new dynamic behavior. Specifically, adding setDynamic() to LOAD_LEVEL
> and TRANSITIVE_COMPILE in Compiler.java

FYI, I've attached an updated patch to CLJ-322:

http://dev.clojure.org/jira/browse/CLJ-322

It applies cleanly to head, addresses the issues raised by Cosmin, and adds some decent tests.

**Note** In the process of writing the tests, I ran into the CLJ-432 bug (due to the hyphen in clojure's test package structure), and fixed that too. So, the patch now on CLJ-432 needs to be applied in order for the tests for CLJ-322 to pass:

http://dev.clojure.org/jira/browse/CLJ-432

Both of these are serious issues that affect everyone, and are worthy of getting a look from many pairs of eyes. If you're reading this, please screen and test both patches, and indicate success or any issues on the relevant ticket(s).

Thanks!

- Chas

Laurent PETIT

unread,
Nov 21, 2010, 7:48:18 AM11/21/10
to cloju...@googlegroups.com


2010/11/20 Chas Emerick <ceme...@snowtide.com>


**Note** In the process of writing the tests, I ran into the CLJ-432 bug (due to the hyphen in clojure's test package structure), and fixed that too.  So, the patch now on CLJ-432 needs to be applied in order for the tests for CLJ-322 to pass:

http://dev.clojure.org/jira/browse/CLJ-432

Both of these are serious issues that affect everyone, and are worthy of getting a look from many pairs of eyes.  If you're reading this, please screen and test both patches, and indicate success or any issues on the relevant ticket(s).


I've tested the patch for CLJ-432 (since I was the reporter of the bug), and I confirm it corrects the bug.
Reply all
Reply to author
Forward
0 new messages