Feature/perf no io #38
Conversation
…es during the median calculation for debug; add default slope intercept values for debug
API to flush umap buffer available pfbenchmark now tests Evictions, Read, Write, WP page faults Added script to drive pfbenchmark
Feature/cfitsio2
|
Sorry, I was misunderstanding Maya's review and deleted the previous comment. As for my median calculation program, I can change my program to run w/o OpenMP if umap also doesn't need it.
Do you think do we still need 'median' and FITS directory? |
| LIBRARY DESTINATION lib | ||
| ARCHIVE DESTINATION lib/static | ||
| RUNTIME DESTINATION bin ) | ||
|
|
||
| else() | ||
| message("Skpping median_calculation, OpenMP required") |
There was a problem hiding this comment.
I am having a difficult time determining which file @mayagokhale is referring to. Is this the file and is "Skpping" the word that needs to be changed? If so, I think this message has been in here since the beginning and we can definitely fix it.
|
@Kieta, if your implementation replaces the old 'median' and FITS directories, then I think we should remove them. |
|
It’s a typo in multiple cmake files. I think Keita found it and has corrected it. Not a big deal, but might as well output error messages with correct spelling.
… On Jun 15, 2018, at 10:51 PM, Marty McFadden ***@***.***> wrote:
@mcfadden8 commented on this pull request.
In tests/median_calculation/CMakeLists.txt:
> LIBRARY DESTINATION lib
ARCHIVE DESTINATION lib/static
RUNTIME DESTINATION bin )
+
else()
message("Skpping median_calculation, OpenMP required")
I am having a difficult time determining which file Maya is talking about. Is this the file and is "Skpping" the word that needs to be changed? If so, I think this message has been in here since the beginning and we can definitely skip it.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Yeah, I see it and am fixing it now. Would like to get this stuff cleaned up too.
After this PR, I will clean this up further (remove FITS directory, only include OMP where it is actually needed, etc.) and then I will submit a separate “clean-up” PR.
Would that be okay?
Marty
|
|
Yes, that sounds good. I’ll try to review the rest of the files in that pull request today.
Maya
… On Jun 16, 2018, at 7:41 AM, Marty McFadden ***@***.***> wrote:
Yeah, I see it and am fixing it now. Would like to get this stuff cleaned up too.
After this PR, I will clean this up further (remove FITS directory, only include OMP where it is actually needed, etc.) and then I will submit a separate “clean-up” PR.
Would that be okay?
Marty
From: mayagokhale ***@***.***>
Reply-To: LLNL/umap ***@***.***>
Date: Saturday, June 16, 2018 at 7:38 AM
To: LLNL/umap ***@***.***>
Cc: "Mcfadden, Marty" ***@***.***>, Mention ***@***.***>
Subject: Re: [LLNL/umap] Feature/perf no io (#38)
It’s a typo in multiple cmake files. I think Keita found it and has corrected it. Not a big deal, but might as well output error messages with correct spelling.
> On Jun 15, 2018, at 10:51 PM, Marty McFadden ***@***.***> wrote:
>
> @mcfadden8 commented on this pull request.
>
> In tests/median_calculation/CMakeLists.txt:
>
> > LIBRARY DESTINATION lib
> ARCHIVE DESTINATION lib/static
> RUNTIME DESTINATION bin )
> +
> else()
> message("Skpping median_calculation, OpenMP required")
>
> I am having a difficult time determining which file Maya is talking about. Is this the file and is "Skpping" the word that needs to be changed? If so, I think this message has been in here since the beginning and we can definitely skip it.
>
> —
> You are receiving this because your review was requested.
> Reply to this email directly, view it on GitHub, or mute the thread.
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#38 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AGgY1dqhQJcjkVJNkUCf97vSimLwPppHks5t9RhugaJpZM4UqOeO>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
There was a problem hiding this comment.
In tests/lib/PerFile/PerFile.cpp lines 123-128, there are little blue squares right before the text. That appears to be the only difference to the old file lines 118-123. What are those characters? tabs?
Also do the regression tests run with these changes merged in?
| perror(estr.c_str()); | ||
| return NULL; | ||
| } | ||
| struct stat sbuf; |
There was a problem hiding this comment.
These lines appear to have extra white space in them. Will investigate/fix.
There was a problem hiding this comment.
Also, the umapsort regression test is indeed passing. However, I also just tried churn and it is not happy. Will investigate as well.
See issue #39. I will add a set of regression test scripts that will be run before each PR. For now, these will need to be run manually until we can find a place where we can configure a test kernel.
There was a problem hiding this comment.
Actually, this part of the diff just shows that these lines had an extra indentation added to them which was intentional because they were moved within a new if block.
| # a single file. As multi-file tests increase (FITS) | ||
| # additional tests will be added. | ||
| # | ||
| churn --directio -f /mnt/intel/regression_test_churn.dat -b 10000 -c 20000 -l 1000 -d 60 |
There was a problem hiding this comment.
@mayagokhale, I have added our first regression test script to the umap project. As soon as I am able to locate a system that we can run our experimental kernel on, we may be able to enable some form of CI.
These two tests are passing now and I'll make sure to run them before submitting subsequent PRs.
2. Fixed read and rmw tests so that reads would not be optimized out by compiler.
Added test date from dst-intel
Added benchmark tests to determine bandwidth of various page fault scenarios
Added APIs to umap for flushing buffer and getting/clearing statistics
Merged in median calculation updates from Keita