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

Remove apache commons deps. #94

Merged
merged 1 commit into from May 18, 2016
Merged

Conversation

andreaTP
Copy link
Contributor

Targeting #22 .

if (f.isDirectory) f.listFiles.map(matchFiles).flatten.toArray
else if (f.getName.endsWith(".nir")) Array(f)
else Array()
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if using arrays is a good idea for recursive construction of a sequence. Lists would probably be a better fit here.

Copy link
Collaborator

@sjrd sjrd May 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A naive use of lists would also be quadratic. But the following is linear:

    def matchFiles(f: File, tail: List[File] = Nil): List[File] = {
      if (f.isDirectory) f.listFiles.foldRight(tail)(matchFiles)
      else if (f.getName.endsWith(".nir")) f :: tail
      else tail
    }

"Typical" difference list. :p

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I didn't think too much at "optimization" at this stage, you guys are crazy :-)

If you really care probably best option is to use java8 File interface with something like:
https://docs.oracle.com/javase/8/docs/api/java/nio/file/Files.html#walk-java.nio.file.Path-java.nio.file.FileVisitOption...-

Too me removing apache commons serves to speed up docker image creation :-) so pick your choice!

Copy link
Member

@densh densh May 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linker is actually a very performance sensitive piece code. I used to have dumb non-lazy linker before and it would take minutes to link. I/O is expensive, even with SSDs. If you make a mistake there, it's amplified many times over proportionally to size of classpath / number of files / dependencies / etc. And those things often get pretty damn huge in practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough :-)

@andreaTP
Copy link
Contributor Author

@densh @sjrd please review this please
Probably there is the need of refactoring string matching methods with the use of Path everywhere

val name = ctor(parts.mkString("."))
(name -> deserializeBinaryFile(fileabs))

matchFiles(baseabs).map {path: Path =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot to run scalafmt.

@densh
Copy link
Member

densh commented May 18, 2016

If you feel like having Paths everywhere would improve the codebase, go for it.

@andreaTP
Copy link
Contributor Author

I run scalafmt, we can leave this PR for the apache/commons fix, and than gradually or within one separate PR refactor to Path all around.
If ok I'll squash

@densh
Copy link
Member

densh commented May 18, 2016

Sounds good to me.

@andreaTP
Copy link
Contributor Author

squashed

@densh
Copy link
Member

densh commented May 18, 2016

Please rebase on top of recent master, and otherwise LGTM.

@andreaTP
Copy link
Contributor Author

should be ok now

@densh densh mentioned this pull request May 18, 2016
@densh
Copy link
Member

densh commented May 18, 2016

LGTM!

@densh densh merged commit 63125a5 into scala-native:master May 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants