Testing bitflag expansion by jsantell · Pull Request #242 · mozilla/mentat

jsantell · 2017-02-03T20:36:52Z

Based off of @ncalexan's #214, relevant commit:

a021735

jsantell · 2017-02-03T20:38:37Z


        // Insert datoms that were added and not already present.
-        let s = r#"
+        let s = format!(r#"


This is performance benchmarking, so someone should have an eye on this query other than me

Yeah, this is early days. We're still deep in the "make it correct" phase, but I was hoping to get some idea of how much this saves. We'll be better able to benchmark as the transactor fleshes out.

ncalexan

This looks great, just like I expected it to. I have many review changes for #214 so this will need rebasing, but it shouldn't be hard!

ncalexan · 2017-02-06T20:45:45Z

 const TRUE: &'static bool = &true;
 const FALSE: &'static bool = &false;

+const INDEX_AVET     : u8 = 1 << 0;


nit: for the final patch, comment on what these are and how the scheme works.

ncalexan · 2017-02-06T20:46:15Z

                // Now we can represent the typed value as an SQL value.
                let (value, value_type_tag): (ToSqlOutput, i32) = typed_value.to_sql_value_pair();

+                let mut flags: u8 = 0;


Consider implementing Attribute.flags() or similar.

ncalexan · 2017-02-06T20:46:54Z

-                                                              .chain(once(to_bool_ref(index_vaet) as &ToSql)
-                                                                     .chain(once(to_bool_ref(index_fulltext) as &ToSql)
-                                                                            .chain(once(to_bool_ref(unique_value) as &ToSql))))))))))
+                                                       .chain(once(flags as &ToSql)))))))


ncalexan · 2017-02-06T20:48:23Z


        // Insert datoms that were added and not already present.
-        let s = r#"
+        let s = format!(r#"


Yeah, this is early days. We're still deep in the "make it correct" phase, but I was hoping to get some idea of how much this saves. We'll be better able to benchmark as the transactor fleshes out.

ncalexan · 2017-02-06T20:49:50Z

@@ -518,10 +523,7 @@ impl DB {
               value_type_tag0 SMALLINT NOT NULL,


These tables definitely deserve a comment about the flag compression. And flags should still be a TINYINT, although this doesn't make much difference in SQLite, IIUC.

jsantell · 2017-02-09T21:01:58Z

@ncalexan rebased on that big PR. For implementing Attribute.flags(), to clarify, are you talking about:

Moving BitFlags type to core/src/lib and importing that into db/src/db
Provide an Attribute.flags() function that returns a bitfield containing all of the flags (index_vaet, etc) that we want in the bitfield?

ncalexan · 2017-02-09T21:24:24Z

Provide an Attribute.flags() function that returns a bitfield containing all of the flags (index_vaet, etc) that we want in the bitfield?

@jsantell, I was thinking of this.

jsantell · 2017-02-10T01:15:53Z

@ncalexan Ok, made the type changes and flags() helper -- good for review

ncalexan

This looks good! I assume that you've looked and you can't define an enum -> u8 conversion better than all the as u8 conversions you've done? Might be worth asking for confirmation from jimb or #rust about how to do this best.

But feel free to land and follow-up if you prefer!

ncalexan · 2017-02-10T18:05:04Z

+pub enum AttributeBitFlags {
+    IndexAVET     = 1 << 0,
+    IndexVAET     = 1 << 1,
+    IndexFullText = 1 << 2,


nit: I think we're considering "fulltext" one word, but I'm not militant about this.

And looks like this is the best way to do this according to #rust

ncalexan · 2017-02-10T18:05:44Z

@@ -55,6 +55,8 @@ pub const CURRENT_VERSION: i32 = 2;
 const TRUE: &'static bool = &true;
 const FALSE: &'static bool = &false;



Spurious change?

ncalexan · 2017-02-10T18:06:25Z

-                    attribute.value_type == ValueType::Ref,
-                    attribute.fulltext,
-                    attribute.unique_value))
+                    flags))


Maybe just inline this call? Your choice, your style.

ncalexan · 2017-02-10T18:09:42Z

Also, check the tests of all the crates -- Travis is angry :)

…n the datoms table to investigate performance difference. Fixes mozilla#226. r=nalexander

jaredhirsch added the in progress label Feb 3, 2017

jsantell commented Feb 3, 2017

View reviewed changes

jsantell requested a review from ncalexan February 3, 2017 20:38

ncalexan reviewed Feb 6, 2017

View reviewed changes

jsantell force-pushed the 226 branch from a021735 to baf0667 Compare February 9, 2017 20:57

jsantell force-pushed the 226 branch 2 times, most recently from 5dd09b2 to b69d426 Compare February 10, 2017 01:15

jsantell force-pushed the 226 branch from b69d426 to 0b54313 Compare February 10, 2017 17:11

ncalexan approved these changes Feb 10, 2017

View reviewed changes

jsantell force-pushed the 226 branch from 0b54313 to c754917 Compare February 10, 2017 19:22

Store a bitfield in temporary search tables and expand to bit flags i…

fe405b5

…n the datoms table to investigate performance difference. Fixes mozilla#226. r=nalexander

jsantell force-pushed the 226 branch from c754917 to fe405b5 Compare February 10, 2017 20:01

jsantell merged commit 6ce5d52 into mozilla:rust Feb 10, 2017

jaredhirsch removed the in progress label Feb 10, 2017

		@@ -518,10 +523,7 @@ impl DB {
		value_type_tag0 SMALLINT NOT NULL,

		@@ -55,6 +55,8 @@ pub const CURRENT_VERSION: i32 = 2;
		const TRUE: &'static bool = &true;
		const FALSE: &'static bool = &false;

Conversation

jsantell commented Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncalexan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsantell commented Feb 9, 2017

Uh oh!

ncalexan commented Feb 9, 2017

Uh oh!

jsantell commented Feb 10, 2017

Uh oh!

ncalexan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncalexan commented Feb 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jsantell commented Feb 3, 2017 •

edited

Loading