Skip to content
This repository has been archived by the owner on Aug 29, 2023. It is now read-only.

Update makefile to run tests #11

Merged
merged 2 commits into from May 11, 2014

Conversation

kanielc
Copy link
Collaborator

@kanielc kanielc commented May 11, 2014

  • Also add expected output file for tests.

The test will generate an actual.txt file and compare that with the
expected output.

I'm not sure where in the README.md you'd want to put an instruction for running the tests, so I left that.

- Also add expected output file for tests.

The test will generate an actual.txt file and compare that with the
expected output.
JossWhittle added a commit that referenced this pull request May 11, 2014
@JossWhittle JossWhittle merged commit 34ac5cd into JossWhittle:master May 11, 2014
@JossWhittle
Copy link
Owner

Brilliant work again, cheers :) Sorry for not pushing too much myself this week work's been a bit hectic. I'll add an initial Build instructions section in the Readme but feel free to add anything you think should be there.

@kanielc
Copy link
Collaborator Author

kanielc commented May 11, 2014

Oh, don't worry about pushing yourself on a side project.

I'm looking to test what Andrei Alexandrescu had asked on reddit.
I have a code base (from work) that is fairly large (40+K LOC).

I just ran Flint++ on it, and it finished in 3.5 seconds
I'm trying to build Flint (painful) and will test it as well.

If Flint wins, then I'll profile Flint++ to see where it's losing time and
optimize it a bit.
I'm not much of a believer in the idea that D leads to faster code than C++.

On Sat, May 10, 2014 at 10:10 PM, Joss Whittle notifications@github.comwrote:

Brilliant work again, cheers :) Sorry for not pushing too much myself this
week work's been a bit hectic. I'll add an initial Build instructions
section in the Readme but feel free to add anything you think should be
there.


Reply to this email directly or view it on GitHubhttps://github.com//pull/11#issuecomment-42760218
.

@JossWhittle
Copy link
Owner

I'd be interested to see those benchmarks. I have a feeling Facebook's D implementation will be significantly faster than their original C++ version, but if anything that will likely be due to overhead in their Folly library on the C++ side.

I've also been toying with the idea of using OpenMP to lint multiple files in parallel. Seeing as all lint messages are buffered in a report object before output there shouldn't be any garbled output.

@kanielc
Copy link
Collaborator Author

kanielc commented May 11, 2014

Couldn't the std::thread and C++11 concurrency model itself allow for that?

I have never used OpenMP, so I may be missing the advantages.

On Sun, May 11, 2014 at 10:03 AM, Joss Whittle notifications@github.comwrote:

I'd be interested to see those benchmarks. I have a feeling Facebook's D
implementation will be significantly faster than their original C++
version, but if anything that will likely be due to overhead in their Folly
library on the C++ side.

I've also been toying with the idea of using OpenMP to lint multiple files
in parallel. Seeing as all lint messages are buffered in a report object
before output there shouldn't be any garbled output.


Reply to this email directly or view it on GitHubhttps://github.com//pull/11#issuecomment-42771277
.

@JossWhittle
Copy link
Owner

my main motivation behind using OpenMP was that I use it frequently for my research so I feel confident in my ability to optimize a program using it.

I've been looking at std::thread and I like the idea of implementing a thread pool to offload lint jobs too as we traverse the file structure and find them. For now my initial OpenMP branch is working quite nicely with only minor additions.

I've refactored the checkEntry function in Main.cpp to be two functions, one which explores the dir structure and puts identified files into a list to be linted, and then a second function which performs the linting and can be parallelized using #pragma omp parallel for

@kevinushey
Copy link

The only downside to using OpenMP is that clang is not distributed on Mac OS X with support for OpenMP, while it does have support for C++11 threads.

That said, OpenMP is supposed to be coming 'soon' for the trunk clang -- it's been done at http://clang-omp.github.io/.

@JossWhittle
Copy link
Owner

Good shout @kevinushey, I didn't realize Clang didn't ship with OpenMP support.

Seems like std::threads is the way to go. Though before parallelizing I think a significant performance boost could be gained if we cluster similar checks into group functions.

Currently every check which (for example) needs to run for each class/struct/union definition all iterate over the whole file to find those initial linting conditions. Similarly, many of the tests iterate over the whole file without being able to skip any tokens like the NULL vs nullptr check. If some of these tests could be grouped into a single pass over the token stream I think it could really speed things up.

@kanielc
Copy link
Collaborator Author

kanielc commented May 11, 2014

So I tested Flint++, Flint C++ and Flint D against our internal project (it's 31K LOC in C++11).
The results were strange:

Flint++ - 0m0.987s Flint C++ - 0m0.391s Flint D - 0m2.047s

I'm not sure why the D version is slower than the C++ versions.
I'll try it on a OSS project and see how it does there.

@JossWhittle
Copy link
Owner

So we're half as fast as their original code. I think this can be partly attributed to their use of a class called StringPiece from their folly framework. It links to a reference to a string and defines a start + end index for the represented substring. For large files the reduction in copy operations for strings could be significant.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants