Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Perfs] x10 on method compiling with #deferFlushDuring: #5292

Conversation

@VincentBlondeau
Copy link
Contributor

VincentBlondeau commented Dec 2, 2019

Changes:

  • Avoid flushing at the end of each chunk.
  • Tools should not read the logfile when the deferFlushing is active because it can read incomplete chunks.

Bench:
Before:

SourceFiles deferFlushDuring: [ [EphemeronTest compile: 'test ^true' ] bench] "'82.668 per second'"

After:

SourceFiles deferFlushDuring: [ [EphemeronTest compile: 'test ^true' ] bench] "'1066.573 per second'"

VB
Tools should not read the logfile when the deferFlushing is active + add test
@tesonep

This comment has been minimized.

Copy link
Collaborator

tesonep commented Dec 2, 2019

Hi Vincent, it looks like the generated source file in the bootstrap does not include the sources.

@svenvc

This comment has been minimized.

Copy link
Contributor

svenvc commented Dec 2, 2019

I saw this excessive flushing before as well, it is obviously too much when writing lots of bits, but it is probably needed when writing only one.

Of course this is again one of those 'let's not touch it because it might be needed', so let's try.

@VincentBlondeau

This comment has been minimized.

Copy link
Contributor Author

VincentBlondeau commented Dec 2, 2019

@tesonep it seems to be more an issue with the statefull traits and Spec

@tesonep

This comment has been minimized.

Copy link
Collaborator

tesonep commented Dec 2, 2019

The problem is that during the installation of a stateful trait the source code of the methods are used (if it requires to recompile, in this case, it is using an slot) so there is a problem with the generation of the changes file

@VincentBlondeau

This comment has been minimized.

Copy link
Contributor Author

VincentBlondeau commented Dec 2, 2019

That's a weird behavior... So either, this mechanism should be changed or the file flushed before compiling the trait. I'll prefer the first but the second sounds easier

@tesonep

This comment has been minimized.

Copy link
Collaborator

tesonep commented Dec 3, 2019

Hi Vincent, I would check the nature of the problem reproducing the bootstrap step by step. I like to integrate your improvement. 😄

@VincentBlondeau

This comment has been minimized.

Copy link
Contributor Author

VincentBlondeau commented Dec 3, 2019

So that the code to reproduce the bug:

compiler := Smalltalk globals at: #FBDDecompiler.
[
	Smalltalk globals removeKey: #FBDDecompiler.
	SourceFiles deferFlushDuring: [ 
	CmTWithBasicNameAndDescription compile: 'test2 ^42'.
	
	CmAbstractCommand subclass: #CmCommand
	uses: CmTWithBasicNameAndDescription + CmTDecorable
	instanceVariableNames: 'context'
	classVariableNames: ''
	package: 'Commander2-Commands'  ].
]ensure: [ Smalltalk globals at: #FBDDecompiler put: compiler ].
CmCommand new test2

In the bootstrap, there is no decompiler so the sources cannot be retrieved from either the source code or the decompiler (through OpalCompiler>>decompileMethod:).

TaAbstratComposition>> #sourceCodeAt: should be implemented through another way to get the data from the other already existing method.

@tesonep

This comment has been minimized.

Copy link
Collaborator

tesonep commented Dec 3, 2019

Hi @VincentBlondeau, I have checked the problem and the problem is that the source code of a method is not accessible until the stream is flushed.

If you check SourceFileArray >>#readStreamAtFileIndex: index atPosition: position ifPresent: presentBlock ifAbsent: absentBlock the code is read from the file directly (using a set of read-only streams directly). I can change this method to read using the w/r stream if they are dirty, although I don't know the impact in speed that will have (as the read-only streams allow Pharo to share the access to the underlying file).

I will try this modification.

Also, I have a doubt, to me, it is not important but maybe someone cares. As if there is an error in loading a metacello package the .changes file is not correctly saved.

@VincentBlondeau

This comment has been minimized.

Copy link
Contributor Author

VincentBlondeau commented Dec 3, 2019

Or you can force the flush of the stream since we not using the statefull traits often. However, I still think the compilation of the traits should not be based on the change files to retrieve the source code in case we have an image in read only mode.

@tesonep

This comment has been minimized.

Copy link
Collaborator

tesonep commented Dec 3, 2019

Yes, the problem it requires the source code (or a decompiled version) to recompile the usage of instance variables or if there is an alias or deep alias.

@tesonep

This comment has been minimized.

Copy link
Collaborator

tesonep commented Dec 3, 2019

#5291 is fixed also with my modification

@VincentBlondeau

This comment has been minimized.

Copy link
Contributor Author

VincentBlondeau commented Dec 5, 2019

Good for me!

@MarcusDenker

This comment has been minimized.

Copy link
Member

MarcusDenker commented Dec 9, 2019

is it good to merge?

@MarcusDenker MarcusDenker requested a review from tesonep Dec 10, 2019
@tesonep tesonep merged commit d9b66e0 into pharo-project:Pharo8.0 Dec 10, 2019
1 of 4 checks passed
1 of 4 checks passed
continuous-integration/benchmarks The benchmarks show regressions.
Details
continuous-integration/jenkins/pr-merge This commit has test failures
Details
probot/minimum-reviews Pending review approvals
WIP Ready for review
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.