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

Logg #1091

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Logg #1091

wants to merge 2 commits into from

Conversation

o-
Copy link
Contributor

@o- o- commented Aug 25, 2021

No description provided.

@o-
Copy link
Contributor Author

o- commented Sep 2, 2021

Here are some stats for your PR:

  • WARNING: the longest CI job test_release_2 took 2.45h
  • real_thing suite improved by 1.03
  • simple suite improved by 1.07
  • summary suite improved by 1.02
  • Bounce regressed by 0.95
  • reversecomplement_naive regressed by 0.94
  • scalarFor regressed by 0.85
  • Overall benchmarks improved by 1.02

Please find your performance results at https://rir-benchmarks.prl.fit.cvut.cz/diff?job_ids[]=1558366701&job_ids[]=1543742924&selection=all

Copy link
Contributor Author

@o- o- left a comment

Choose a reason for hiding this comment

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

Hey, thanks a lot for this. I am looking forward to finally try it out for real.

I added some comments that you can consider if you like. One general comment: we have a clang-format post-commit hook and make a habit of just blindly following its suggested format. The PR seems to have some lines where the formatting is off. Maybe you did not have clang-format installed? The hook only applies to changed lines. So the trick (for retroactively fixing the formatting) is to soft-reset git and then create one commit for the whole change. At that point the pre-commit hook should automatically format all the changes. Let me know if you need any help with that.

rir/src/api.cpp Outdated
@@ -34,6 +37,8 @@ static bool oldPreserve = false;
static unsigned oldSerializeChaos = false;
static bool oldDeoptChaos = false;

std::unordered_map<int, std::set<unsigned long>> conMap;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you give this a more descriptive name and maybe a comment. I am not sure what it does

rir/src/api.cpp Outdated
@@ -580,6 +644,32 @@ REXPORT SEXP rirCreateSimpleIntContext() {
}

bool startup() {
if (getenv("BLACKLIST") != NULL) { // load blacklisted contexts
Copy link
Contributor Author

Choose a reason for hiding this comment

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

would you mind calling this a blocklist instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe add a small comment of what this list does?

}
}

bool isBlacklisted(const Context & c) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

you could keep the list sorted, then you can abort the search once your entry is larger than the key?

@@ -21,6 +21,7 @@ typedef SEXP DispatchTableEntry;
struct DispatchTable
: public RirRuntimeObject<DispatchTable, DISPATCH_TABLE_MAGIC> {

int hast;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

call it astHash?

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.

2 participants