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

SQLite benchmark #113

Merged
merged 18 commits into from
Jul 9, 2024
Merged

SQLite benchmark #113

merged 18 commits into from
Jul 9, 2024

Conversation

m-atalla
Copy link
Contributor

Hi,

This PR adds SQLite Amalgamation as a benchmark to test IR2Vec performance.

@svkeerthy I have a couple of points that I'm not sure about:

  1. Should this benchmark be conditionally enabled with a flag?
  2. Is it OK to leave the SQLite source files in the build directory?

Let me know if you have any additional suggestions.

Thanks,
Mohamed

@svkeerthy
Copy link
Member

Hi @m-atalla,

Thanks for the PR!

  1. Yes, it would be good to conditionally enable it. It is fine to have it ON by default.
  2. It is fine to leave the source in the build directory under test directory.

Additionally, I think your PR would be a good to value addition to the functional tests as well. This can be done by linking it with lit and test for the expected embeddings, just like what we currently do for PE-Benchmarks. Please let me know if you have any questions.

Thanks,
Venkat

@m-atalla m-atalla marked this pull request as draft July 2, 2024 11:21
@m-atalla
Copy link
Contributor Author

m-atalla commented Jul 2, 2024

Thanks for your feedback @svkeerthy.

I'm not very familiar with how lit works here, it looks like a simple test runner. Anyhow, I think doing tests function and program level tests are rather straight forward. However, onDemand function level seems bit more involved as I'll have to extract all functions of the SQLite.

Unfortunately, I don't have much free time this week, so I have turned this PR into a draft for now, will open it again and ping you once this is ready.

@m-atalla m-atalla marked this pull request as ready for review July 5, 2024 21:35
@m-atalla
Copy link
Contributor Author

m-atalla commented Jul 5, 2024

@svkeerthy

@svkeerthy
Copy link
Member

Hi @m-atalla,

Apologies for the delay. I was caught up with some deadlines.

Thanks for the changes. The code looks good. Weirdly, I could not see "Test Passed" or "Test Failed" logs for SQLite in the tests action eventhough the ENABLE_SQLITE option is set to ON. Am I missing something?

@m-atalla
Copy link
Contributor Author

m-atalla commented Jul 8, 2024

Correct, the tests status should be reported in the runner, it looks like the IR file is being generated correctly. Let me double check and verify that its working as intended.

@m-atalla
Copy link
Contributor Author

m-atalla commented Jul 8, 2024

@svkeerthy this is fixed, the test action is working correctly now.

Sorry I had some cached build files that made this hard to debug on my end. Anyways, All I needed was to re-order the CMake commands so SQLite stuff is generated before the test script is configured and the benchmarks files are copied.

I think next I can look into profiling so that we know what portions of IR2Vec could benefit the most from parallelizing. I will share my findings in #101 by next Friday if I find anything worth sharing.

@svkeerthy
Copy link
Member

Great! Sounds good.

@svkeerthy svkeerthy merged commit dd38800 into IITH-Compilers:main Jul 9, 2024
2 checks passed
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