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.

Sign up for GitHub

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

Jump to bottom

abstraction above the file system for the compiler. #4053

Closed
wants to merge 1 commit into from
Closed

abstraction above the file system for the compiler. #4053

wants to merge 1 commit into from

Conversation

RIg410
Copy link
Contributor

@RIg410 RIg410 commented May 26, 2020

Implements the abstraction above the file system for the compiler.
Fixed memory leak.

Motivation

We use the compiler as a service. For distributed compilation purposes. We are prevented by leak_str and the fact that the compiler uses files as a source. It would be nice to have abstraction above the file system.

Have you read the Contributing Guidelines on pull requests?

yes.

Test Plan

Unit test.

Related PRs

--

@RIg410 RIg410 changed the title Implements the abstraction above the file system for the compiler. abstraction above the file system for the compiler. May 26, 2020
@MIRAI-bot
Copy link

A wild Move coverage report has appeared!

New coverage report 
+-------------------------+
| Move Coverage Summary   |
+-------------------------+
Module 00000000000000000000000000000000::Debug
>>> % Module coverage: NaN
Module 00000000000000000000000000000000::Signer
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Testnet
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraTimestamp
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Association
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::AccountLimits
>>> % Module coverage: 96.87
Module 00000000000000000000000000000000::Vector
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Empty
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::AccountType
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::VASP
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Unhosted
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::AccountTrack
>>> % Module coverage: 88.96
Module 00000000000000000000000000000000::LCS
>>> % Module coverage: NaN
Module 00000000000000000000000000000000::Hash
>>> % Module coverage: NaN
Module 00000000000000000000000000000000::Authenticator
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Offer
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Event
>>> % Module coverage: 92.25
Module 00000000000000000000000000000000::LibraConfig
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::RegisteredCurrencies
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::FixedPoint32
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Libra
>>> % Module coverage: 98.78
Module 00000000000000000000000000000000::Coin1
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Coin2
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Compare
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Option
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Signature
>>> % Module coverage: NaN
Module 00000000000000000000000000000000::LibraTransactionTimeout
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LBR
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraAccount
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::ValidatorConfig
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraSystem
>>> % Module coverage: 98.98
Module 00000000000000000000000000000000::TransactionFee
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraWriteSetManager
>>> % Module coverage: 14.94
Module 00000000000000000000000000000000::LibraBlock
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Genesis
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraVMConfig
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraVersion
>>> % Module coverage: 84.38
Module 00000000000000000000000000000000::SharedEd25519PublicKey
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::SlidingNonce
>>> % Module coverage: 85.31
+-------------------------+
| % Move Coverage: 97.11  |
+-------------------------+
Baseline coverage report 
+-------------------------+
| Move Coverage Summary   |
+-------------------------+
Module 00000000000000000000000000000000::Debug
>>> % Module coverage: NaN
Module 00000000000000000000000000000000::Testnet
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraTimestamp
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Association
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::AccountLimits
>>> % Module coverage: 96.87
Module 00000000000000000000000000000000::Vector
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Empty
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::AccountType
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::VASP
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Unhosted
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::AccountTrack
>>> % Module coverage: 88.96
Module 00000000000000000000000000000000::LCS
>>> % Module coverage: NaN
Module 00000000000000000000000000000000::Hash
>>> % Module coverage: NaN
Module 00000000000000000000000000000000::Authenticator
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Offer
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Event
>>> % Module coverage: 92.25
Module 00000000000000000000000000000000::LibraConfig
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::RegisteredCurrencies
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::FixedPoint32
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Libra
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Coin1
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Coin2
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Compare
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Option
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Signature
>>> % Module coverage: NaN
Module 00000000000000000000000000000000::LibraTransactionTimeout
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraAccount
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::ValidatorConfig
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraSystem
>>> % Module coverage: 98.98
Module 00000000000000000000000000000000::TransactionFee
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraWriteSetManager
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraBlock
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LBR
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Genesis
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraVMConfig
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraVersion
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::SharedEd25519PublicKey
>>> % Module coverage: 100.00
+-------------------------+
| % Move Coverage: 99.14  |
+-------------------------+

If these two differ, you should look at updating the baseline, or adding more tests.

You can get more details here

@MIRAI-bot
Copy link

A wild Move coverage report has appeared!

New coverage report 
+-------------------------+
| Move Coverage Summary   |
+-------------------------+
Module 00000000000000000000000000000000::Debug
>>> % Module coverage: NaN
Module 00000000000000000000000000000000::Signer
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Testnet
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraTimestamp
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Association
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::AccountLimits
>>> % Module coverage: 96.87
Module 00000000000000000000000000000000::Vector
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Empty
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::AccountType
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::VASP
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Unhosted
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::AccountTrack
>>> % Module coverage: 88.96
Module 00000000000000000000000000000000::LCS
>>> % Module coverage: NaN
Module 00000000000000000000000000000000::Hash
>>> % Module coverage: NaN
Module 00000000000000000000000000000000::Authenticator
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Offer
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Event
>>> % Module coverage: 92.25
Module 00000000000000000000000000000000::LibraConfig
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::RegisteredCurrencies
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::FixedPoint32
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Libra
>>> % Module coverage: 98.78
Module 00000000000000000000000000000000::Coin1
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Coin2
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Compare
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Option
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Signature
>>> % Module coverage: NaN
Module 00000000000000000000000000000000::LibraTransactionTimeout
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LBR
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraAccount
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::ValidatorConfig
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraSystem
>>> % Module coverage: 98.98
Module 00000000000000000000000000000000::TransactionFee
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraWriteSetManager
>>> % Module coverage: 14.94
Module 00000000000000000000000000000000::LibraBlock
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Genesis
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraVMConfig
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraVersion
>>> % Module coverage: 84.38
Module 00000000000000000000000000000000::SharedEd25519PublicKey
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::SlidingNonce
>>> % Module coverage: 85.31
+-------------------------+
| % Move Coverage: 97.11  |
+-------------------------+
Baseline coverage report 
+-------------------------+
| Move Coverage Summary   |
+-------------------------+
Module 00000000000000000000000000000000::Debug
>>> % Module coverage: NaN
Module 00000000000000000000000000000000::Testnet
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraTimestamp
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Association
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::AccountLimits
>>> % Module coverage: 96.87
Module 00000000000000000000000000000000::Vector
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Empty
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::AccountType
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::VASP
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Unhosted
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::AccountTrack
>>> % Module coverage: 88.96
Module 00000000000000000000000000000000::LCS
>>> % Module coverage: NaN
Module 00000000000000000000000000000000::Hash
>>> % Module coverage: NaN
Module 00000000000000000000000000000000::Authenticator
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Offer
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Event
>>> % Module coverage: 92.25
Module 00000000000000000000000000000000::LibraConfig
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::RegisteredCurrencies
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::FixedPoint32
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Libra
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Coin1
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Coin2
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Compare
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Option
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Signature
>>> % Module coverage: NaN
Module 00000000000000000000000000000000::LibraTransactionTimeout
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraAccount
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::ValidatorConfig
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraSystem
>>> % Module coverage: 98.98
Module 00000000000000000000000000000000::TransactionFee
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraWriteSetManager
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraBlock
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LBR
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Genesis
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraVMConfig
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraVersion
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::SharedEd25519PublicKey
>>> % Module coverage: 100.00
+-------------------------+
| % Move Coverage: 99.14  |
+-------------------------+

If these two differ, you should look at updating the baseline, or adding more tests.

You can get more details here

@MIRAI-bot
Copy link

A wild Move coverage report has appeared!

New coverage report 
+-------------------------+
| Move Coverage Summary   |
+-------------------------+
Module 00000000000000000000000000000000::Debug
>>> % Module coverage: NaN
Module 00000000000000000000000000000000::Signer
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Testnet
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraTimestamp
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Association
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::AccountLimits
>>> % Module coverage: 96.87
Module 00000000000000000000000000000000::Vector
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Empty
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::AccountType
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::VASP
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Unhosted
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::AccountTrack
>>> % Module coverage: 88.96
Module 00000000000000000000000000000000::LCS
>>> % Module coverage: NaN
Module 00000000000000000000000000000000::Hash
>>> % Module coverage: NaN
Module 00000000000000000000000000000000::Authenticator
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Offer
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Event
>>> % Module coverage: 92.25
Module 00000000000000000000000000000000::LibraConfig
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::RegisteredCurrencies
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::FixedPoint32
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Libra
>>> % Module coverage: 98.78
Module 00000000000000000000000000000000::Coin1
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Coin2
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Compare
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Option
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Signature
>>> % Module coverage: NaN
Module 00000000000000000000000000000000::LibraTransactionTimeout
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LBR
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraAccount
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::ValidatorConfig
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraSystem
>>> % Module coverage: 98.98
Module 00000000000000000000000000000000::TransactionFee
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraWriteSetManager
>>> % Module coverage: 14.94
Module 00000000000000000000000000000000::LibraBlock
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Genesis
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraVMConfig
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraVersion
>>> % Module coverage: 84.38
Module 00000000000000000000000000000000::SharedEd25519PublicKey
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::SlidingNonce
>>> % Module coverage: 85.31
+-------------------------+
| % Move Coverage: 97.11  |
+-------------------------+
Baseline coverage report 
+-------------------------+
| Move Coverage Summary   |
+-------------------------+
Module 00000000000000000000000000000000::Debug
>>> % Module coverage: NaN
Module 00000000000000000000000000000000::Testnet
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraTimestamp
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Association
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::AccountLimits
>>> % Module coverage: 96.87
Module 00000000000000000000000000000000::Vector
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Empty
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::AccountType
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::VASP
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Unhosted
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::AccountTrack
>>> % Module coverage: 88.96
Module 00000000000000000000000000000000::LCS
>>> % Module coverage: NaN
Module 00000000000000000000000000000000::Hash
>>> % Module coverage: NaN
Module 00000000000000000000000000000000::Authenticator
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Offer
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Event
>>> % Module coverage: 92.25
Module 00000000000000000000000000000000::LibraConfig
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::RegisteredCurrencies
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::FixedPoint32
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Libra
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Coin1
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Coin2
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Compare
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Option
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Signature
>>> % Module coverage: NaN
Module 00000000000000000000000000000000::LibraTransactionTimeout
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraAccount
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::ValidatorConfig
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraSystem
>>> % Module coverage: 98.98
Module 00000000000000000000000000000000::TransactionFee
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraWriteSetManager
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraBlock
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LBR
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::Genesis
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraVMConfig
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::LibraVersion
>>> % Module coverage: 100.00
Module 00000000000000000000000000000000::SharedEd25519PublicKey
>>> % Module coverage: 100.00
+-------------------------+
| % Move Coverage: 99.14  |
+-------------------------+

If these two differ, you should look at updating the baseline, or adding more tests.

You can get more details here

@sblackshear sblackshear requested review from dariorussi-zz, vgao1996, tnowacki and runtian-zhou and removed request for vgao1996 May 26, 2020 22:57
tnowacki
tnowacki reviewed May 26, 2020
View reviewed changes
Copy link
Contributor

@tnowacki tnowacki left a comment

Choose a reason for hiding this comment

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

Thanks the PR! We definitely want to do something like this eventually.
We will try to hash out if this is the right solution at this time, but I want to make sure we don't kick the can down the road into having to find another solution if anything around the source language changes in the future

language/move-core/types/src/fs.rs
Comment on lines +16 to +18
thread_local! {
static DESCRIPTORS: RefCell<Option<Descriptors>> = RefCell::new(None);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Before I go too deep into the PR. I'd like to hammer out some details in this file.

The reason I didn't initially do something like this is that at some point in the future, the Move source language compiler will want to be parallel/multi-threaded. At that time, I was worried any such implementation for interned file strings would fall apart.

How will this work if the Move source language were to become multithreaded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now the implementation cannot work in a multithreaded environment. But it is quite easy to fix,
we can do something like that.

static DESCRIPTORS: Lazy<RwLock<Option<Descriptors>>> = Lazy::new(|| RwLock::new(None));

#[derive(Debug)]
struct Descriptors {
    /// Context counter.
    fs_counter: usize,
    /// Path table.
    paths: Vec<String>,
}

The table will be created together with the context at the beginning of compilation and destroyed at the end.
This will avoid memory leaks.
This is the main goal of our PR.
I would like to discuss ways to solve this problem.

runtian-zhou
runtian-zhou reviewed May 26, 2020
View reviewed changes
language/move-core/types/src/fs.rs
// Copyright (c) The Libra Core Contributors
// SPDX-License-Identifier: Apache-2.0

use serde::{de, export::Formatter, Deserialize, Deserializer, Serialize, Serializer};
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to avoid adding this file to move-core. The reason is that it sits relatively low in libra's dependency tree, meaning that all crates in libra will basically depend on this crate. I would argue that moving this to move-lang seems to be a better location.

Copy link
Contributor

Choose a reason for hiding this comment

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

The source language and the IR use the same spanned information. So maybe in the IR types, move-ir/types?

@tnowacki
Copy link
Contributor

tnowacki commented Jun 3, 2020

Taking a longer look at this, I don't think this is the right change at this time.

Quick summarized thoughts:

  • Currently, the leaking isn't a big issue at the moment as the Move source language compiler is not intended to be hooked into any long lived process. It should only be invoked via a command line tool, and should not be integrated into another Rust service
  • Long term, we will need to include some sort of intern table for these strings. That is: the Box::leak solution is an intentional, temporary solution that allows iteration on the compiler to move faster
  • In the future, we will move the string table (along with other compiler instance specific information) inside of a context that is threaded through the compiler.
    • There is a lot of work needed to get to that point though, so it isn't likely something that can be done in the next few weeks

I appreciate the PR in that it addresses a known issue in the compiler, but it is not the solution we want to take.
In the future, I think it will be more helpful to first open a GitHub issue before making a PR. It will give you a better sense of how we are thinking about addressing these problems, and let us figure out the best solution going forward.
Nonetheless, I really do appreciate the effort put in here! These changes aren't quick or easy to do, and I love seeing external contributions for the source language. But right now, we are heavily focused on getting this first version fully working, so while I do love seeing them, our bandwidth might be limited for dealing with external contributions.

@tnowacki tnowacki closed this Jun 3, 2020
@RIg410 RIg410 deleted the feature-source-pool branch August 5, 2020 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tnowacki tnowacki tnowacki left review comments

@runtian-zhou runtian-zhou runtian-zhou left review comments

@dariorussi-zz dariorussi-zz Awaiting requested review from dariorussi-zz

@vgao1996 vgao1996 Awaiting requested review from vgao1996

Assignees
No one assigned
Labels
cla-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@RIg410 @MIRAI-bot @tnowacki @runtian-zhou