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

Experiment to switch to nanoserde instead of serde #860

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion crates/rustc_codegen_spirv-types/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,4 @@ repository = "https://github.com/EmbarkStudios/rust-gpu"

[dependencies]
rspirv = "0.11"
serde = { version = "1.0", features = ["derive"] }
nanoserde = "0.1.9"
20 changes: 9 additions & 11 deletions crates/rustc_codegen_spirv-types/src/compile_result.rs
Original file line number Diff line number Diff line change
@@ -1,17 +1,15 @@
use serde::{Deserialize, Serialize};
use std::collections::BTreeMap;
use nanoserde::{DeJson, SerJson};
use std::collections::HashMap;
use std::fmt::Write;
use std::path::{Path, PathBuf};

#[derive(Debug, Serialize, Deserialize)]
#[serde(untagged)]
#[derive(Debug, DeJson, SerJson)]
pub enum ModuleResult {
SingleModule(PathBuf),
MultiModule(BTreeMap<String, PathBuf>),
SingleModule(String),
MultiModule(HashMap<String, String>),
Copy link
Contributor

Choose a reason for hiding this comment

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

Check if using a HashMap here doesn't impact buildtime determinism.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point. I think it's likely best to just stick to BTreeMap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nanoserde doesn't support BTreeMap unfortunately, that's why I changed it.

but yes logically it should be a BTreeMap here. though the builds are rather non-deterministic anyway so probably quite fine. but should comment it

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it would be pretty easy to add BTreeMap to nanoserde if we wanted to, just saying. But yeah we can stick to hashmap while we evaluate.

Choose a reason for hiding this comment

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

nanoserde has a closed issue on this here: not-fl3/nanoserde#7
Apparently the owner of the library prefers to use a proxy in this case

[...] instead of adding all possible containers into the library. "

Might be worthwhile to just open a PR over there if the additional buildtime determinism is worth it.

}

impl ModuleResult {
pub fn unwrap_single(&self) -> &Path {
pub fn unwrap_single(&self) -> &String {
match self {
ModuleResult::SingleModule(result) => result,
ModuleResult::MultiModule(_) => {
Expand All @@ -20,7 +18,7 @@ impl ModuleResult {
}
}

pub fn unwrap_multi(&self) -> &BTreeMap<String, PathBuf> {
pub fn unwrap_multi(&self) -> &HashMap<String, String> {
match self {
ModuleResult::MultiModule(result) => result,
ModuleResult::SingleModule(_) => {
Expand All @@ -30,7 +28,7 @@ impl ModuleResult {
}
}

#[derive(Debug, Serialize, Deserialize)]
#[derive(Debug, DeJson, SerJson)]
pub struct CompileResult {
pub module: ModuleResult,
pub entry_points: Vec<String>,
Expand All @@ -48,7 +46,7 @@ impl CompileResult {
#[derive(Default)]
struct Trie<'a> {
present: bool,
children: BTreeMap<&'a str, Trie<'a>>,
children: HashMap<&'a str, Trie<'a>>,
}

impl<'a> Trie<'a> {
Expand Down
3 changes: 1 addition & 2 deletions crates/rustc_codegen_spirv/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,7 @@ indexmap = "1.6.0"
rspirv = "0.11"
rustc-demangle = "0.1.21"
sanitize-filename = "0.3"
serde = { version = "1.0", features = ["derive"] }
serde_json = "1.0"
nanoserde = "0.1.9"
smallvec = "1.6.1"
spirv-tools = { version = "0.8", default-features = false }
rustc_codegen_spirv-types = { path = "../rustc_codegen_spirv-types", version = "0.4.0-alpha.12" }
Expand Down
38 changes: 21 additions & 17 deletions crates/rustc_codegen_spirv/src/decorations.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
//! SPIR-V decorations specific to `rustc_codegen_spirv`, produced during
//! the original codegen of a crate, and consumed by the `linker`.

#![allow(clippy::question_mark)] // warning from inside the nanoserde macro

use nanoserde::{DeJson, SerJson};
use rspirv::dr::{Instruction, Module, Operand};
use rspirv::spirv::{Decoration, Op, Word};
use rustc_span::{source_map::SourceMap, FileName, Pos, Span};
use serde::{Deserialize, Serialize};
use std::marker::PhantomData;
use std::path::PathBuf;
use std::{iter, slice};
use std::{marker::PhantomData, path::Path};

/// Decorations not native to SPIR-V require some form of encoding into existing
/// SPIR-V constructs, for which we use `OpDecorateString` with decoration type
Expand All @@ -22,15 +23,15 @@ use std::{iter, slice};
///
/// TODO: uses `non_semantic` instead of piggybacking off of `UserTypeGOOGLE`
/// <https://htmlpreview.github.io/?https://github.com/KhronosGroup/SPIRV-Registry/blob/master/extensions/KHR/SPV_KHR_non_semantic_info.html>
pub trait CustomDecoration: for<'de> Deserialize<'de> + Serialize {
pub trait CustomDecoration: DeJson + SerJson {
const ENCODING_PREFIX: &'static str;

fn encode(self, id: Word) -> Instruction {
// FIXME(eddyb) this allocates twice, because there is no functionality
// in `serde_json` for writing to something that impls `fmt::Write`,
// only for `io::Write`, which would require performing redundant UTF-8
// (re)validation, or relying on `unsafe` code, to use with `String`.
let json = serde_json::to_string(&self).unwrap();
let json = self.serialize_json();
let encoded = [Self::ENCODING_PREFIX, &json].concat();

Instruction::new(
Expand Down Expand Up @@ -93,27 +94,26 @@ pub struct LazilyDeserialized<'a, D> {
_marker: PhantomData<D>,
}

impl<'a, D: Deserialize<'a>> LazilyDeserialized<'a, D> {
impl<'a, D: DeJson> LazilyDeserialized<'a, D> {
pub fn deserialize(self) -> D {
serde_json::from_str(self.json).unwrap()
D::deserialize_json(self.json).unwrap()
}
}

/// An `OpFunction` with `#[spirv(unroll_loops)]` on the Rust `fn` definition,
/// which should get `LoopControl::UNROLL` applied to all of its loops'
/// `OpLoopMerge` instructions, during structuralization.
#[derive(Deserialize, Serialize)]
#[derive(DeJson, SerJson)]
pub struct UnrollLoopsDecoration {}

impl CustomDecoration for UnrollLoopsDecoration {
const ENCODING_PREFIX: &'static str = "U";
}

#[derive(Deserialize, Serialize)]
#[derive(DeJson, SerJson)]
pub struct ZombieDecoration {
pub reason: String,

#[serde(flatten)]
pub span: Option<SerializedSpan>,
}

Expand All @@ -124,9 +124,9 @@ impl CustomDecoration for ZombieDecoration {
/// Representation of a `rustc` `Span` that can be turned into a `Span` again
/// in another compilation, by reloading the file. However, note that this will
/// fail if the file changed since, which is detected using the serialized `hash`.
#[derive(Deserialize, Serialize)]
#[derive(DeJson, SerJson)]
pub struct SerializedSpan {
file: PathBuf,
file: String,
hash: serde_adapters::SourceFileHash,
lo: u32,
hi: u32,
Expand All @@ -135,9 +135,9 @@ pub struct SerializedSpan {
// HACK(eddyb) `rustc_span` types implement only `rustc_serialize` traits, but
// not `serde` traits, and the easiest workaround is to have our own types.
mod serde_adapters {
use serde::{Deserialize, Serialize};
use nanoserde::{DeJson, SerJson};

#[derive(Copy, Clone, PartialEq, Eq, Deserialize, Serialize)]
#[derive(Copy, Clone, PartialEq, Eq, DeJson, SerJson)]
pub enum SourceFileHashAlgorithm {
Md5,
Sha1,
Expand All @@ -154,7 +154,7 @@ mod serde_adapters {
}
}

#[derive(Copy, Clone, PartialEq, Eq, Deserialize, Serialize)]
#[derive(Copy, Clone, PartialEq, Eq, DeJson, SerJson)]
pub struct SourceFileHash {
kind: SourceFileHashAlgorithm,
value: [u8; 32],
Expand Down Expand Up @@ -197,7 +197,11 @@ impl SerializedSpan {
file: match &file.name {
// We can only support real files, not "synthetic" ones (which
// are almost never exposed to the compiler backend anyway).
FileName::Real(real_name) => real_name.local_path()?.to_path_buf(),
FileName::Real(real_name) => real_name
.local_path()?
.to_path_buf()
.to_string_lossy()
.to_string(),
_ => return None,
},
hash: file.src_hash.into(),
Expand All @@ -207,7 +211,7 @@ impl SerializedSpan {
}

pub fn to_rustc(&self, source_map: &SourceMap) -> Option<Span> {
let file = source_map.load_file(&self.file).ok()?;
let file = source_map.load_file(Path::new(&self.file)).ok()?;

// If the file has changed since serializing, there's not much we can do,
// other than avoid creating invalid/confusing `Span`s.
Expand Down
11 changes: 6 additions & 5 deletions crates/rustc_codegen_spirv/src/link.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::codegen_cx::{CodegenArgs, ModuleOutputType, SpirvMetadata};
use crate::{linker, SpirvCodegenBackend, SpirvModuleBuffer, SpirvThinBuffer};
use ar::{Archive, GnuBuilder, Header};
use nanoserde::SerJson;
use rspirv::binary::Assemble;
use rustc_ast::CRATE_NODE_ID;
use rustc_codegen_spirv_types::{CompileResult, ModuleResult};
Expand All @@ -19,7 +20,7 @@ use rustc_session::Session;
use std::env;
use std::ffi::CString;
use std::fs::File;
use std::io::{BufWriter, Read};
use std::io::Read;
use std::iter;
use std::path::{Path, PathBuf};
use std::sync::Arc;
Expand Down Expand Up @@ -151,7 +152,8 @@ fn link_exe(
module_filename.push("module");
post_link_single_module(sess, &cg_args, spv_binary.assemble(), &module_filename);
cg_args.do_disassemble(&spv_binary);
let module_result = ModuleResult::SingleModule(module_filename);
let module_result =
ModuleResult::SingleModule(module_filename.to_string_lossy().to_string());
CompileResult {
module: module_result,
entry_points: entry_points(&spv_binary),
Expand All @@ -170,7 +172,7 @@ fn link_exe(
spv_binary.assemble(),
&module_filename,
);
(name, module_filename)
(name, module_filename.to_string_lossy().to_string())
})
.collect();
let module_result = ModuleResult::MultiModule(map);
Expand All @@ -181,8 +183,7 @@ fn link_exe(
}
};

let file = File::create(out_filename).unwrap();
serde_json::to_writer(BufWriter::new(file), &compile_result).unwrap();
std::fs::write(out_filename, compile_result.serialize_json()).unwrap();
}

fn entry_points(module: &rspirv::dr::Module) -> Vec<String> {
Expand Down
4 changes: 2 additions & 2 deletions crates/rustc_codegen_spirv/src/linker/peephole_opts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@ fn process_instruction(
}

/// Fuse a sequence of scalar operations into a single vector operation. For example:
/// ```
/// ```ignore
/// %x_0 = OpCompositeExtract %x 0
/// %x_1 = OpCompositeExtract %x 1
/// %y_0 = OpCompositeExtract %y 0
Expand All @@ -410,7 +410,7 @@ fn process_instruction(
/// %r = OpCompositeConstruct %r_0 %r_1
/// ```
/// into
/// ```
/// ```ignore
/// %r = OpAdd %x %y
/// ```
/// (We don't remove the intermediate instructions, however, in case they're used elsewhere - we
Expand Down
4 changes: 2 additions & 2 deletions crates/rustc_codegen_spirv/src/linker/specializer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
//! "generic" parameters to every (indirect) mention of any pointer type in
//! the group, using an additional parameter remapping, for which `Generic`:
//! * requires this extra documentation:
//! ```
//! ```ignore
//! /// The one exception are `OpTypePointer`s involved in recursive data types
//! /// (i.e. they were declared by `OpTypeForwardPointer`s, and their pointees are
//! /// `OpTypeStruct`s that have the same pointer type as a leaf).
Expand All @@ -36,7 +36,7 @@
//! /// a mapping (`expand_params`) indicates how to create the flattened list.
//! ```
//! * and this extra field:
//! ```
//! ```ignore
//! /// For every entry in the regular flattened list of parameters expected by
//! /// operands, this contains the parameter index (i.e. `0..self.param_count`)
//! /// to use for that parameter.
Expand Down
3 changes: 1 addition & 2 deletions crates/spirv-builder/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,7 @@ watch = ["notify"]
[dependencies]
memchr = "2.4"
raw-string = "0.3.5"
serde = { version = "1.0", features = ["derive"] }
serde_json = "1.0"
nanoserde = "0.1.9"
rustc_codegen_spirv-types = { path = "../rustc_codegen_spirv-types" }
# See comment in lib.rs invoke_rustc for why this is here
rustc_codegen_spirv = { path = "../rustc_codegen_spirv", default-features = false }
Expand Down
Loading