Team 5 new language design by qianxichen233 · Pull Request #15 · nyuoss/ninja-shadowdash-nyu

qianxichen233 · 2024-10-26T08:58:50Z

This PR addresses the new language design task for Team 5.

Our team’s new language design has already been integrated into our Shadowdash application rather than existing as an independent design, which is therefore restricted by a few practical challenges:

Container Choice: We currently use std::vector instead of std::initializer_list because, while initializer_list supports constexpr computations, it is not intended to function as a container by design. To make the application work, we have switched to vector. Currently, we are still investigating constexpr-friendly containers that could maximize compile-time computations during the shared object compilation process.
Compilation Time Issues: We recently encountered an extreme slowdown in compilation time when building our new language-based manifest into the shared object file, especially as the number of build objects and strings grew to a very high levels. For example, compiling manifest.cc (translated from LLVM’s build.ninja) includes over 20k build objects and millions of strings, which imposes such a heavy processing load on both clang++ and g++ that the compilation time has become impractical, taking over 10 hours without completion. To address this, we experimented with limiting the scope of objects to prevent Clang from generating unnecessary DAGs. While this approach works, it requires further changes to the language design and shifts some compile-time overhead to runtime, which we'd prefer to avoid. We are still to exploring solutions for this issue, and we may or may not revise the current language design based on our findings. This is why these potential changes are not yet reflected in this PR.

yw5490 · 2024-11-03T00:40:47Z

Code Review From @cchels @395110n @yw5490

Please include more comments to explain the code for better readability.
It's better to break changes into a few smaller commits of appropriate size, rather than just one commit that includes everything.
The code is well-structured.
The absence of detailed testing and sufficient documentation for new language features poses risks to stability and maintainability.
The macros defined provide enough detail on the APIs and how to use these macros, but could provide more comments on how to use the APIs.
Generally, the design for the new language is good, and the style is consistent, and the functionally is working appropriately.

cchels · 2024-11-25T00:35:47Z

+	);
+
+	// Define a build configuration for compiling "hello.c" into object files.
+	auto build1 = build(list(str("hello.o"), str("hello2.o")),


For build1, build2 and build3, these names are generic and do not convey the purpose of the builds. Consider renaming them to reflect the specific target, like buildHelloObjects.

cchels · 2024-11-25T00:38:27Z

+    std::string value_;
+};
+
+Token operator"" _l(const char* value, std::size_t len) {


For operator"" _l and operator"" _v, while these are clever, the naming might not be immediately clear to someone unfamiliar with the codebase. Consider adding inline comments explaining what these operators do, or using more expressive suffixes than _l and _v.

yw5490 · 2024-11-25T00:49:22Z

Code Review From @cchels @395110n @yw5490

Change Granularity

1 commit per PR perfectly meets the requirements indicated in lecture.
All the files included in the commit are relevant.

Code Functionality

The code constructs for build and rule definitions are in line with what appears in ninja manual.
Console pool is missing.
All the classes have consistent structure with clear logic flow.

Readability & Structure

Check in-line comments for feedback about naming conventions.
The functions are encapsulated well.

Standards Compatibility

Currently, there seems to be no linting error, but should set up Circle CI to use clang-tidy and include a link of the test result to prove no linting error.
Class fields should be private, so that data is not exposed publicly to the user.

Testing

Should set up Circle CI to test the new language design portion.
Should include a link to the CircleCI page showing that all tests have passed.

yw5490 · 2024-11-25T00:41:40Z

+public:
+    ShadowDash_(std::vector<build> builds) : builds(builds), pools({}), default_({}) {}
+    ShadowDash_(std::vector<build> builds, std::vector<pool> pools) : builds(builds), pools(pools), default_({}) {}
+    ShadowDash_(std::vector<build> builds, std::vector<pool> pools, shadowdash_default default_) : builds(builds), pools(pools), default_({default_}) {}


This line is too long, should split this line into two.

kyotov

take a look at https://docs.google.com/document/d/11e8zR3kcm7B-D6brAH_D4OgtYBEf94GZwQIXrnti5hM/edit?tab=t.0 and see if you are missing any of those. then let me know to look again.

this looks "ok" but i get lost in it a bit.

notably design doc / cleaner api could help?

kyotov · 2024-11-27T02:33:48Z

why this symlink

this new language design is an old version, i.e. very first version of the new language. In this version, we are still doing the graph construction in ninja, and only let shadowdash manifest constructs the build object. So the manifest.h is required by both manifest.cc and ninja, which is the reason I am using symlink here.
The newest language design already moved the entire graph construction into the manifest, so ninja no longer require manifest.h and this symlink could go away

kyotov

see above

update manifest Add comments and documentation to shadowdash_manifest.h & manifest_example.cc added documentation for shadowdash

qianxichen233 · 2024-12-04T22:21:57Z

Update: added a document under shadowdash_docs folder explaining the api design, use case, etc.

Honestly, I feel like new language design is something that is not supposed to happen at this stage of development, or at least a carefully, comprehensive new language design that we spent a lot of times on it. The main reason is that, I believe shadowdash would prioritize performance over usability (i.e. user experience), but when designing a new language as a seperate module without integrating into a fully working shadowdash, like what we did here, we are not able to measure the performance, and thus our new language design is basically purely based on usability. This become an issue when we are trying to really integrate the new language into the backend, that our current new language design might be really bad in performance and requires us to re-design the new language to make the compilation/runtime faster. In our newest shadowdash demo, this is exactly what is happening, that our newest new language design that gains most performance benefits is much more different from the original one that is proposed in this PR, as we have integrated tricks like string variable pre-defining, multiple build constructor for different build object combination, manifest file spliting, etc. And I believe our new language design would be even more different as we move forward to apply more tricks to resolve performance issue. This involves very frequently change of new language design and I believe it would be a waste of time to try to fully document each version and design things for usability since many of the stuff might go away very fast as more iterations on new language design happens. The way I believe makes more sense to me for new language design, is that, we first propose a minimal version of new language design that could just work, then we can focus more on optimizing the performance of the compilation and runtime, which might involves significant change of the new language design. After all the performance tricks are consolidated and we got the performance we might want, and we can probably release the version, we can then start working on the usability part of the new language design and try to make the new language design that is maximized in performance more user friendly and well documented for people to use.

qianxichen233 · 2024-12-13T04:37:46Z

Moved to #31

qianxichen233 force-pushed the new-language-design branch from c838e32 to a2f0f68 Compare October 30, 2024 23:06

qianxichen233 force-pushed the new-language-design branch from cebec61 to 42d74a5 Compare November 20, 2024 23:16

cchels reviewed Nov 25, 2024

View reviewed changes

yw5490 reviewed Nov 25, 2024

View reviewed changes

kyotov reviewed Nov 27, 2024

View reviewed changes

kyotov requested changes Nov 27, 2024

View reviewed changes

added new language design

4558c50

update manifest Add comments and documentation to shadowdash_manifest.h & manifest_example.cc added documentation for shadowdash

qianxichen233 force-pushed the new-language-design branch from 42d74a5 to 4558c50 Compare December 4, 2024 22:21

qianxichen233 closed this Dec 13, 2024

Navigation Menu

Uh oh!

Team 5 new language design - #15

Team 5 new language design#15
qianxichen233 wants to merge 1 commit into
nyuoss:masterfrom
qianxichen233:new-language-design

qianxichen233 commented Oct 26, 2024

Uh oh!

yw5490 commented Nov 3, 2024

Uh oh!

cchels Nov 25, 2024

Uh oh!

cchels Nov 25, 2024

Uh oh!

yw5490 commented Nov 25, 2024

Uh oh!

yw5490 Nov 25, 2024

Uh oh!

kyotov left a comment

Uh oh!

kyotov Nov 27, 2024

Uh oh!

qianxichen233 Dec 4, 2024

Uh oh!

kyotov left a comment

Uh oh!

qianxichen233 commented Dec 4, 2024

Uh oh!

qianxichen233 commented Dec 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

qianxichen233 commented Oct 26, 2024

Uh oh!

yw5490 commented Nov 3, 2024

Uh oh!

cchels Nov 25, 2024

Choose a reason for hiding this comment

Uh oh!

cchels Nov 25, 2024

Choose a reason for hiding this comment

Uh oh!

yw5490 commented Nov 25, 2024

Uh oh!

yw5490 Nov 25, 2024

Choose a reason for hiding this comment

Uh oh!

kyotov left a comment

Choose a reason for hiding this comment

Uh oh!

kyotov Nov 27, 2024

Choose a reason for hiding this comment

Uh oh!

qianxichen233 Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

kyotov left a comment

Choose a reason for hiding this comment

Uh oh!

qianxichen233 commented Dec 4, 2024

Uh oh!

qianxichen233 commented Dec 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants